D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik added inline comments.

INLINE COMMENTS

> zzag wrote in abstract_egl_backend.cpp:427
> What's holding us from doing that?

We would need to create a separate EGL image and a separate texture for each 
plane.
The scene would need to bind each of those textures to separate texture binding 
points, and we would need to generate and use a shader that samples texels from 
each plane and performs YUV to RGB conversion.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: romangg, #kwin, #plasma, davidedmundson, mart, graesslin, fredrik
Cc: meven, zzag, romangg, anthonyfieroni, plasma-devel, kwin, LeGast00n, 
sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, 
Pitel, iodelay, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

fredrik added a comment.


  In D10747#237235 , @romangg wrote:
  
  > Regarding the "drm_fourcc.h" file: do we want to copy it in KWayland's code 
base or could we use the system one? It's only available on Linux? In this case 
could we include it as a dummy only on non-Linunx systems? This way we could 
use the system one on Linux.
  
  
  I don't know if it's available on all platforms we support, and copying it 
was the path of least resistance.
  
  > Should LinuxDmabufParams subclass Resource?
  
  I considered doing that, but Resource is designed in such a way that it 
requires the use of a d-pointer, and LinuxDmabufParams is a private class.

INLINE COMMENTS

> romangg wrote in linuxdmabuf_v1_interface.h:107
> I'm not sure what's the canonical way in KWayland to do this. I assume the 
> supported formats and modifiers could be saved in a field of the interface's 
> Private class on creation.
> 
> The buffer could be imported through a signal to the compositor and a 
> function in the interface to be called by the compositor afterwards to 
> proceed.

I considered that, but it seems ugly to have the compositor call back into 
KWayland from the slot. It would also be necessary to pass a pointer to the 
LinuxDmabufParams object in a signal parameter, just so the compositor can pass 
it back to KWayland.

A slightly less ugly option would to make the LinuxDmabufParams class public, 
and emit the signal from the params object. But that would require the 
compositor to track the creation of those objects so it can connect the signal 
to a slot.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10747

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, ngraham, bruns, alexeymin, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, eliasp, sebas, apol, mart, hein

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik updated this revision to Diff 29953.
fredrik added a comment.


  Fix issues pointed out by romangg.

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=29331=29953

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/backend.cpp
  platformsupport/scenes/opengl/backend.h
  plugins/scenes/opengl/scene_opengl.cpp
  plugins/scenes/opengl/scene_opengl.h
  scene.cpp
  scene.h
  wayland_server.cpp
  wayland_server.h

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: romangg, anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik added a comment.


  In D10750#224860 , @romangg wrote:
  
  > In D10750#216337 , @fredrik 
wrote:
  >
  > > An issue that this patch does not fully address is switching compositing 
backends at runtime.
  >
  >
  > Do we support that at all? The backend is set at startup. Don't think you 
can change this later on.
  
  
  That is the question I'm asking.

INLINE COMMENTS

> romangg wrote in abstract_egl_backend.h:100
> Why QLinkedList? It should be no better than QList for the removeOne call. 
> For this better use QSet.

I believe the use of QList is discouraged in new code, but you are right about 
QSet being a better choice in this case.

> romangg wrote in abstract_egl_backend.h:135
> Why is it necessary to export?

I guess it isn't. I was thinking at the time that someone might want to use 
this class in a class derived from AbstractEglBackend or AbstractEglTexture.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: romangg, anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10251: [RFC] Use mipmap filtering in window thumbnails

[Fredrik Höglund]

fredrik updated this revision to Diff 29832.
fredrik added a comment.


  Rebase on master.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10251?vs=26374=29832

REVISION DETAIL
  https://phabricator.kde.org/D10251

AFFECTED FILES
  src/declarativeimports/core/windowthumbnail.cpp
  src/declarativeimports/core/windowthumbnail.h

To: fredrik, #plasma, #vdg
Cc: progwolff, broulik, ngraham, hein, plasma-devel, #frameworks, michaelh, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10251: [RFC] Use mipmap filtering in window thumbnails

[Fredrik Höglund]

fredrik added a comment.


  In D10251#227092 , @broulik wrote:
  
  > What's the state of this? Bug 391915 just cropped up
  
  
  There are still some issues with the cubic filter, and I've been distracted 
by other things lately.
  I think we should consider landing the current version in the meantime, 
because like @ngraham said, it is a big visual win even in its current state.
  
  It would also be nice to get some testing before the release.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D10251

To: fredrik, #plasma, #vdg
Cc: broulik, ngraham, hein, plasma-devel, #frameworks, michaelh, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11262: KDE logout screen background color fix

[Fredrik Höglund]

fredrik added inline comments.

INLINE COMMENTS

> mart wrote in Logout.qml:87
> the method lattes is using 0.2126*r + 0.7152*g + 0.0722*b is more correct
> (a simple one would be convertin to gray scale with  qGray, but unfortunately 
> is C++ only)

qGray() uses an integer approximation that is only correct for CRT monitors.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D11262

To: Pitel, mart, #plasma
Cc: fredrik, ngraham, abetts, broulik, mvourlakos, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik updated this revision to Diff 29331.
fredrik added a comment.


  Import the context.

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10750?vs=27803=29331

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/backend.cpp
  platformsupport/scenes/opengl/backend.h
  plugins/scenes/opengl/scene_opengl.cpp
  plugins/scenes/opengl/scene_opengl.h
  scene.cpp
  scene.h
  wayland_server.cpp
  wayland_server.h

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

fredrik updated this revision to Diff 28986.
fredrik added a comment.


  Import the context.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10747?vs=27735=28986

REVISION DETAIL
  https://phabricator.kde.org/D10747

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, schernikov, michaelh, ZrenBot, 
alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik added a comment.


  An issue that this patch does not fully address is switching compositing 
backends at runtime.
  
  Buffers are imported by the scene. The scene allocates and returns a 
subclassed LinuxDmabuf::Buffer that contains the file descriptors and backend 
specific objects, such as EGL images. When the compositing backend is 
destroyed, the EGL images are also destroyed, but the buffers survive. The new 
backend could use the file descriptors to re-import the buffers, if the buffer 
class wasn't specific to the old backend.
  
  A possible solution to this is a shared Buffer class (in 
platformsupport/scenes/common?):
  
...
private:
...
QVector planes;
EGLimage eglImage;
VkImage vulkanImage;
VkImageView imageView;
VkDeviceMemory deviceMemory;
};
  
  Or a SceneBufferData object, created and destroyed by the scene:
  
private:
...
QVector planes;
SceneBufferData *sceneData;
};
  
  But even with a shared class there are no guarantees that a re-import is 
possible, since the new backend might not support the same formats and/or 
modifiers. So I don't know if runtime switching is something that should be 
expected to work.

INLINE COMMENTS

> anthonyfieroni wrote in abstract_egl_backend.cpp:117
> Call delete dmabuf will have same effect.

No, it will not. It will leave a dangling pointer in the wl_resource, which 
will result in a double-free when the client deletes the buffer. Or a segfault 
the next time it tries to attach the buffer to a surface.

I will expand on this in a separate comment.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: anthonyfieroni, plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik edited projects, added Plasma; removed KWin.
fredrik added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  In D10750#211784 , @graesslin 
wrote:
  
  > Concerning the tests: the ones requiring OpenGL work best if module vgem is 
loaded. That normally makes them pass. The tests regarding keyboard layout need 
env variable XDG_DEFAULT_LAYOUT being unset or on us.
  
  
  Those results are with vgem loaded, and XDG_DEFAULT_LAYOUT is not set.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: plasma-devel, kwin, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol, mart

D10750: wayland: Add support for zwp_linux_dmabuf

[Fredrik Höglund]

fredrik created this revision.
fredrik added reviewers: KWin, Plasma, davidedmundson, mart, graesslin.
Restricted Application added a project: KWin.
Restricted Application added subscribers: kwin, plasma-devel.
fredrik requested review of this revision.
Restricted Application edited projects, added Plasma; removed KWin.

REVISION SUMMARY
  This adds support for LinuxDmabufUnstableV1Interface in kwin.

TEST PLAN
  I asked Marco to test it with a driver that supports modifiers, and he 
confirmed that it works.
  I have also checked that kwin still works with drivers that don't support 
modifiers.
  
  Test results before (6 failures):
  
  The following tests FAILED:
  
30 - kwin-testLockScreen (Failed)
39 - kwin-testPointerInput (Failed)
57 - kwin-testSceneOpenGL-waylandonly (Failed)
71 - kwin-testKeyboardLayout (Failed)
74 - kwin-testKeymapCreationFailure-waylandonly (Failed)
88 - kwin-testColorCorrectNightColor-waylandonly (Failed)
  
  Test results after (8 failures):
  
  The following tests FAILED:
  
30 - kwin-testLockScreen (Failed)
39 - kwin-testPointerInput (Failed)
59 - kwin-testSceneOpenGLES-waylandonly (Failed)
60 - kwin-testNoXdgRuntimeDir (Failed)
61 - kwin-testNoXdgRuntimeDir-waylandonly (Failed)
71 - kwin-testKeyboardLayout (Failed)
72 - kwin-testKeyboardLayout-waylandonly (Failed)
74 - kwin-testKeymapCreationFailure-waylandonly (Failed)
  
  I'm not sure what to make of this, but at least some of these failures are 
spurious.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10750

AFFECTED FILES
  platformsupport/scenes/opengl/abstract_egl_backend.cpp
  platformsupport/scenes/opengl/abstract_egl_backend.h
  platformsupport/scenes/opengl/backend.cpp
  platformsupport/scenes/opengl/backend.h
  plugins/scenes/opengl/scene_opengl.cpp
  plugins/scenes/opengl/scene_opengl.h
  scene.cpp
  scene.h
  wayland_server.cpp
  wayland_server.h

To: fredrik, #kwin, #plasma, davidedmundson, mart, graesslin
Cc: plasma-devel, kwin, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

fredrik added inline comments.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

INLINE COMMENTS

> linuxdmabuf_v1_interface.h:39
> +
> +namespace LinuxDmabuf
> +{

Do we want these nested namespaces? We could have LinuxDmabufFlags, 
LinuxDmabufBuffer etc. instead.

> linuxdmabuf_v1_interface.h:65
> + */
> +class Buffer {
> +public:

Should the Buffer class use a d-pointer?

> linuxdmabuf_v1_interface.h:107
> + */
> +class Bridge {
> +public:

Is this the solution we want for interfacing with the compositor?

My preference would be to use std::function callbacks, with setters in 
LinuxDmabufUnstableV1Interface. Setting up the interface could then look like 
this:

  m_linuxDmabuf = m_display->createLinuxDmabufInterface(m_display);
  m_linuxDmabuf->setQuerySupportedFormats([]{ return 
Compositor::self()->scene()->supportedDrmFormats(); });
  ...
  m_linuxDmabuf->create();

This can also be extended without breaking binary compatibility. But I don't 
think we can use std::function in frameworks. There are also BIC issues when 
mixing different STL implementations, which we may or may not care about.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10747

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, schernikov, alexeymin, eliasp, hein

D10747: Implement zwp_linux_dmabuf_v1

[Fredrik Höglund]

fredrik created this revision.
fredrik added reviewers: KWin, Plasma, graesslin, davidedmundson, mart.
Restricted Application added projects: Plasma on Wayland, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.
fredrik requested review of this revision.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REVISION SUMMARY
  This interface provides a way for clients to create generic dmabuf-based 
wl_buffers.

TEST PLAN
  I asked Marco to test it with a driver that supports modifiers, and he says 
that it works.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10747

AFFECTED FILES
  src/client/protocols/linux-dmabuf-unstable-v1.xml
  src/server/CMakeLists.txt
  src/server/buffer_interface.cpp
  src/server/buffer_interface.h
  src/server/display.cpp
  src/server/display.h
  src/server/drm_fourcc.h
  src/server/linuxdmabuf_v1_interface.cpp
  src/server/linuxdmabuf_v1_interface.h

To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D10181: Removed the "fastblur" path

[Fredrik Höglund]

This revision was automatically updated to reflect the committed changes.
Closed by commit R108:f8ff40271e4d: Removed the fastblur path 
(authored by anemeth, committed by fredrik).
Restricted Application edited projects, added Plasma; removed KWin.

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D10181?vs=26279=26452#toc

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10181?vs=26279=26452

REVISION DETAIL
  https://phabricator.kde.org/D10181

AFFECTED FILES
  effects/blur/blur.cpp
  effects/blur/blur.h
  effects/blur/blur.kcfg
  effects/blur/blur_config.ui
  effects/logout/data/1.10/logout-blur.frag
  effects/logout/data/1.40/logout-blur.frag
  effects/shaders.qrc

To: anemeth, graesslin, #kwin, #plasma, #vdg, fredrik
Cc: avaragic, fredrik, ngraham, plasma-devel, kwin, #kwin, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10181: Removed the "fastblur" path

[Fredrik Höglund]

fredrik accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added KWin; removed Plasma.

REPOSITORY
  R108 KWin

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10181

To: anemeth, graesslin, #kwin, #plasma, #vdg, fredrik
Cc: avaragic, fredrik, ngraham, plasma-devel, kwin, #kwin, iodelay, bwowk, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, 
sebas, apol, mart

D10251: [RFC] Use mipmap filtering in window thumbnails

[Fredrik Höglund]

fredrik added a comment.


  Before and after:
  F5688790: before-after.png 

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D10251

To: fredrik, #plasma, #vdg
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10251: [RFC] Use mipmap filtering in window thumbnails

[Fredrik Höglund]

fredrik created this revision.
fredrik added reviewers: Plasma, VDG.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.
fredrik requested review of this revision.

REVISION SUMMARY
  Blit the contents of the window texture to a separate mipmap texture, and 
(re)generate the mipmaps on each damage event.
  
  The mipmap filter is the default linear filter, which results in soft images. 
Using a filter with a sharpening kernel, such as a cubic or a windowed sinc 
filter would produce better results, but this is still an improvement over not 
using a mipmap filter at all.
  
  The scaling is gamma-correct when desktop GL is used.
  
  Note that this patch only modifies the GLX path.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D10251

AFFECTED FILES
  src/declarativeimports/core/windowthumbnail.cpp
  src/declarativeimports/core/windowthumbnail.h

To: fredrik, #plasma, #vdg
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10170: Added optional transparency/blur to menu frames

[Fredrik Höglund]

fredrik added a comment.


  > I think the "blur" option should go. Blur should be controlled centrally by 
the desktop effect. In other words: BlurBehind should always be set to true, 
and then left to kwin to handle. Having an extra option here seems like 
micro-management. Why would you need blur behind plasma widgets (as handled by 
the desktop effect) and not behind menus ? (or vice versa). Also, right now, 
selecting "blur" in the style settings, but unselecting the desktop effect 
results in no blur behind menu, and hence inconsistency with what the style 
option says.
  
  KWin has no way of knowing which pixels should be blurred behind a window. 
That's the reason the hint exists.
  If you set the hint unconditionally, the effect will be applied 
unconditionally. Even when the menus are fully opaque.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D10170

To: anemeth, hpereiradacosta, #plasma, colomar, alake
Cc: fredrik, alake, januz, abetts, colomar, andreask, zzag, ngraham, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
sebas, apol, mart

D10181: Removed the "fastblur" path

[Fredrik Höglund]

fredrik added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  Code wise this LGTM.
  
  Consider it accepted if the VDG has no objections.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10181

To: anemeth, graesslin, #kwin, #plasma
Cc: fredrik, ngraham, plasma-devel, kwin, #kwin, iodelay, bwowk, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, 
apol, mart

D10181: Removed the "fastblur" path

[Fredrik Höglund]

fredrik added inline comments.
Restricted Application edited projects, added Plasma; removed KWin.

INLINE COMMENTS

> blur.kcfg:9
>  
> -5
> -
> -
> -false
> +10
>  

This looks like an unrelated change.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D10181

To: anemeth, graesslin, #kwin, #plasma
Cc: fredrik, ngraham, plasma-devel, kwin, #kwin, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10173: windowthumbnail: Fix the GLXFBConfig selection code

[Fredrik Höglund]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R242:46121d58484c: windowthumbnail: Fix the GLXFBConfig 
selection code (authored by fredrik).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10173?vs=26165=26249

REVISION DETAIL
  https://phabricator.kde.org/D10173

AFFECTED FILES
  src/declarativeimports/core/windowthumbnail.cpp
  src/declarativeimports/core/windowthumbnail.h

To: fredrik, #plasma
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10173: windowthumbnail: Fix the GLXFBConfig selection code

[Fredrik Höglund]

fredrik created this revision.
fredrik added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.
fredrik requested review of this revision.

REVISION SUMMARY
  Check that the sizes of the color channels in the fbconfig match those
  in the window visual.
  

  
  This fixes a 2-10-10-10 fbconfig being choosen for the ARGB32 visual.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D10173

AFFECTED FILES
  src/declarativeimports/core/windowthumbnail.cpp
  src/declarativeimports/core/windowthumbnail.h

To: fredrik, #plasma
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Fredrik Höglund]

fredrik accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma; removed KWin.

REPOSITORY
  R108 KWin

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9848

To: anemeth, #plasma, #kwin, fredrik
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson, 
fredrik, ngraham, plasma-devel, kwin, #kwin, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Fredrik Höglund]

fredrik added inline comments.
Restricted Application edited projects, added KWin; removed Plasma.

INLINE COMMENTS

> kwinglutils.cpp:1104
> +
> +s_renderTargets.reserve(s_renderTargets.size() + targets.size());
> +

The reserve() call should be in the else {} clause.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D9848

To: anemeth, #plasma, #kwin
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson, 
fredrik, ngraham, plasma-devel, kwin, #kwin, iodelay, bwowk, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, 
apol

D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Fredrik Höglund]

fredrik added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  I haven't looked at everything in detail, but in general I think this looks 
ready to land.
  
  Just a couple of nitpicks, and a question below.

INLINE COMMENTS

> anemeth wrote in blur.cpp:145
> Ok I realize now that the extension name begins with GL_ and this is the 
> reason it didn't find them.
> BUT then I listed all the available extensions and since they are in 
> alphabetical order I can easily check if texture_barrier is available.
> F5662968: Képkivágás2.PNG 
> The red arrow is where they should be but they aren't present.
> So my point still stands.

> GL_ARB_texture_barrier is supported since OpenGL 4.4, but KWin uses 3.0
>  GL_NV_texture_barrier is supported since OpenGL 3.0 but it only works on 
> Nvidia right?
>  What about AMD and Intel GPUs then?

The 3.1 backend can and does use some GL 4 extensions, but it must have a 
fallback path for when they are not available.

The NV prefix only tells you which vendor created the extension, not which 
vendors support it. The AMD drivers in Mesa have supported texture_barrier 
since 2011, and the i965 driver has supported it since 2015.

But as I said on IRC, don't worry about it if your driver doesn't support it. 
We can add a path that uses texture_barrier after this has landed.

> blur.cpp:255
> +foreach (EffectWindow *window, effects->stackingOrder()) {
> +window->addRepaintFull();
> +}

Does calling effects->addRepaintFull() also work?

> blur.cpp:30
>  #include 
> +#include  // for ceil()
>  

Nitpick, but it should be  and std::ceil() in C++.

> blur.cpp:190
> + *
> + * The minOffset variable is the minimum offset value for an iteratoin 
> before we
> + * get blocky artifacts because of the downsampling.

Typo: iteratoin

> kwinglutils.cpp:1115
> +targets.removeFirst();
> +}
> +

You could also check if s_renderTargets is empty, and just do s_renderTargets = 
targets when that's the case.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D9848

To: anemeth, #plasma, #kwin
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson, 
fredrik, ngraham, plasma-devel, kwin, #kwin, iodelay, bwowk, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, 
apol

D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Fredrik Höglund]

fredrik added inline comments.
Restricted Application edited projects, added KWin; removed Plasma.

INLINE COMMENTS

> blur.cpp:30
>  #include 
> +#include 
>  

Is this still needed when qPow() is not used?

> anemeth wrote in blur.cpp:145
> This texture is only used by `copyScreenSampleTexture()`
> It could very well be a separate variable, but I just appended another 
> texture to the textures vector and used that instead.
> The reason for first rendering into m_renderTextures.last() and then copying 
> that to m_renderTextures[0] is to eliminate what I call "extended blur".
> Extended blur is when windows or other elements that are not under the 
> blurred area affect the blur effect. This sounds great in theory, and this is 
> how the old blur method worked as well, but it becomes a big issue when the 
> blur radius gets big. For example when you maximize a white window, the 
> completely transparent taskbar also becomes almost completely white because 
> of this effect, so the way I achieve this is by creating a GL_CLAMP_TO_EDGE 
> effect.
> F5656755: c3_clamping.png 
> For example if we want to blur a window (red) we have to blur a bigger area. 
> To avoid extended blur I use the copySample shader to create a 
> GL_CLAMP_TO_EDGE effect (blue) when copying the texture from 
> m_renderTextures.last() to m_renderTextures[0]
> 
> Ideally we could specify to only disable extended blur on the taskbar, but I 
> don't see a way to identify a window as the taskbar.

Okay, I see what you mean. I think this is a trade-off between two undesirable 
effects though, because the clamping causes abrupt changes near the edges of 
the blurred region when a window is moved. You can see this effect pretty 
clearly in the video you attached when the blur strength is set to strong. It 
really comes down to what you think is worse though.

But my concern here is that a fullscreen texture consumes at least 8 MB of VRAM 
with an HD monitor, and 32 MB with a 4K monitor.

I think it would be better to copy the framebuffer to m_renderTexture[0], and 
apply the clamping as a post-processing effect, using m_renderTexture[0] as 
both the source and destination, and targeting only the region outside the red 
rectangle for rendering. Using the same texture as both the source and 
destination is allowed in OpenGL when GL_ARB_texture_barrier or 
GL_NV_texture_barrier is supported, but unfortunately not in OpenGL ES.

The taskbar (and other panels) can be identified by calling 
EffectWindow::isDock().

> anemeth wrote in blur.h:41
> I don't know why it's 5
> This was in the old blur too: 
> https://github.com/KDE/kwin/blob/master/effects/blur/blur.cpp#L503
> I just move this number to a variable.

It looks like that was added by Martin in 
https://phabricator.kde.org/R108:111db93e05a55496e7f13788207ead008bac80db.

> fredrik wrote in blurshader.h:53
> The first letter in enums should be capitalized. I think SampleType would 
> also be a better name.

The first letters in the enumerators should also be capitalized.

> kwinglutils.h:477
> + **/
> +static void setRenderTargets(QStack  targets);
> +

This assumes that s_renderTargets is empty when this function is called, which 
might not be the case.
How about naming it pushRenderTargets(), and have it add the targets to 
s_renderTargets instead of replacing it?

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D9848

To: anemeth, #plasma, #kwin
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson, 
fredrik, ngraham, plasma-devel, kwin, #kwin, iodelay, bwowk, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, 
apol

D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Fredrik Höglund]

fredrik added a comment.
Restricted Application edited projects, added Plasma; removed KWin.


  Don't forget to add your name to the license headers.

INLINE COMMENTS

> blur.cpp:46
>  m_simpleShader = 
> ShaderManager::instance()->generateShaderFromResources(ShaderTrait::MapTexture,
>  QString(), QStringLiteral("logout-blur.frag"));
> +m_simpleTarget = new GLRenderTarget(GLTexture(GL_RGBA8, 1, 1));
> +

You should add a GLRenderTarget constructor that takes no arguments.
This is working around deficiencies in the API.

> blur.cpp:116
> +return !target->valid();
> +}) == m_renderTargets.cend();
> +}

I suggest doing this check after creating the render targets, and caching the 
result.

> blur.cpp:135
> +deleteFBOs();
> +
> +for (int i = 0; i <= m_downSampleIterations; i++) {

Call reserve() on m_renderTextures and m_renderTargets here.

> blur.cpp:137
> +for (int i = 0; i <= m_downSampleIterations; i++) {
> +m_renderTextures.append(GLTexture(GL_RGBA8, 
> effects->virtualScreenSize() / qPow(2, i)));
> +m_renderTextures.last().setFilter(GL_LINEAR);

1 << i

> blur.cpp:145
> +// This last set is used as a temporary helper texture
> +m_renderTextures.append(GLTexture(GL_RGBA8, 
> effects->virtualScreenSize()));
> +m_renderTextures.last().setFilter(GL_LINEAR);

Why is this needed?

I'm probably missing something here, but it looks to me as if the effect copies 
the contents of the framebuffer to the helper texture, then copies the contents 
of that texture to m_renderTextures[0], after which the contents of helper 
texture is not used again. Can't copyScreenSampleTexture() copy directly from 
the framebuffer to m_renderTextures[0]?

> blur.cpp:694
> +m_simpleTarget->attachTexture(blurTexture);
> +m_simpleTarget->blitFromFramebuffer(w->geometry(), QRect(QPoint(0, 0), 
> w->size()));
>  

Detach the texture from the render target before you return from this method - 
otherwise the FBO continues to hold a reference to the texture, preventing it 
from being deleted.

> blur.cpp:123
> +{
> +for (int i = 0; i < m_renderTargets.size(); i++) {
> +delete m_renderTargets[i];

You can use qDeleteAll() here.

> blur.cpp:127
> +
> +m_renderTextures[i].discard();
> +}

There is no need to call discard on the textures - just clear the vector.

> blur.cpp:374
> +for (int i = 0; i <= downSampleIterations; i++) {
> +const int divisionRatio = qPow(2, i);
> +

1 << i

> blur.cpp:717
> +vbo->draw(GL_TRIANGLES, blurRectCount * i, blurRectCount);
> +GLRenderTarget::popRenderTarget();
> +}

These three lines of code will expand to an unfortunate sequence of GL calls:

  // Iteration N
  glGetIntegerv(GL_VIEWPORT, ...);
  glBindFramebuffer(GL_FRAMEBUFFER, ...);
  glViewport(...);
  glDrawArrays(...);
  glBindFramebuffer(GL_FRAMEBUFFER, 0);
  glViewport(...);
  
  // Iteration N+1
  glGetIntegerv(GL_VIEWPORT, ..);
  glBindFramebuffer(GL_FRAMEBUFFER, ...);
  glViewport(...);
  glDrawArrays(...);
  glBindFramebuffer(GL_FRAMEBUFFER, 0);
  glViewport(...);

Note the redundant calls to glViewport() and glBindFramebuffer(). The worst 
offender here is glGetIntegerv() however, because it forces serialization of 
the internal driver threads.

Ideally the call sequence should look like this:

  // Iteration N
  glBindFramebuffer(GL_FRAMEBUFFER, ...);
  glViewport(...);
  glDrawArrays(...);
  
  // Iteration N+1
  glBindFramebuffer(GL_FRAMEBUFFER, ...);
  glViewport(...);
  glDrawArrays(...);

This can be fixed in a followup patch though.

> blur.h:41
>  
> +static const int borderSize = 5;
> +

This number could use an explanation.

> blurshader.cpp:241
> +"uniform float offset;\n"
> +"uniform vec2 textureSize;\n";
> +

textureSize is also the name of a built-in function in GLSL, so I suggest 
changing the name to targetSize, renderTargetSize, framebufferSize or something 
similar. That also makes it clear that it is not the size of the texture being 
sampled.

> blurshader.h:95
> +GLShader *m_shaderUpsample = nullptr;
> +GLShader *m_shaderCopysample = nullptr;
> +

I think you could simplify the code quite a bit by having an array of

  struct {
  GLShader *shader;
  int mvpMatrixLocation;
  ...
  };

and use m_activeSampleType as an index into that array.

> blurshader.h:47
> +virtual void setOffset(float offset) = 0;
> +virtual void setTextureSize(QSize textureSize) = 0;
> +virtual void setBlurRect(QRect blurRect, QSize screenSize) = 0;

I suggest changing the name to setTargetSize() to make it clear that the size 
is the size of the render target, and not the size of the texture being sampled.

> blurshader.h:50
>  
> -virtual void bind() = 0;
> +virtual void bind(int sampleType) = 0;
>  virtual void unbind() = 0;

sampleTypeEnum sampleType

> blurshader.h:53
> +
> +enum sampleTypeEnum {
> +downSampleType,

The first 

D6565: Query supported OpenGL core profile version on X11, GLX

[Fredrik Höglund]

fredrik added a comment.


  You should use the GLX_MESA_query_renderer extension for this when it is 
available.

REPOSITORY
  R102 KInfoCenter

REVISION DETAIL
  https://phabricator.kde.org/D6565

To: madcatx, graesslin
Cc: fredrik, graesslin, cfeck, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

Re: KWin 5.10.3.1 update

[Fredrik Höglund]

On Saturday 08 July 2017, Martin Flöser wrote:
> Am 2017-07-07 15:54, schrieb Maximiliano Curia:
> > ¡Hola Jonathan!
> > 
> > El 2017-07-03 a las 17:26 +0100, Jonathan Riddell escribió:
> >> KWin has received an update to the 5.10.3 release. 5.10.3.1 contains a 
> >> fix for https://bugs.kde.org/show_bug.cgi?id=381870 Freeze in 
> >> KWin::checkGLError on startup which affects machines with an NVidia 
> >> card.
> > 
> >> The change is 
> >> https://commits.kde.org/kwin/aefb5f4dd9d41aa7377d56ece203089c73aefe07
> > 
> >> https://www.kde.org/info/plasma-5.10.3.php
> > 
> >> kwin-5.10.3.1 4.4MB 
> >> 199e3a2593e9e66bbd6521ee8a25a012003d15f6b4bf2f102c70b798c9abd03a
> > 
> > Is this fix backportable to the 5.8 branch?
> 
> NO! The fix is for a regression introduced with 5.10.3.
> 
> If there are changes which can be backported we will do. Please do not 
> cherry pick changes, that makes it impossible for us to ensure the 
> quality or investigate bugs.

Technically it is not a regression.  It is just more likely to happen in
5.10.3, but it affects all versions of KWin.

KWin can also crash when a graphics reset has occurred, since it assumes
that glMapBufferRange() never returns NULL, and that is not the case when
the context has been lost.

Fredrik

D5245: Desaturate non-responsive windows

[Fredrik Höglund]

fredrik added a comment.


  In https://phabricator.kde.org/D5245#98926, @broulik wrote:
  
  > > Lower brightness?
  >
  > I actually increase the brightness to produce this washed-out effect but 
admittedly it doesn't look as good in less graphically intensive apps (I used 
Gwenview in the screenshot where you can tell easily but in Kwrite you probably 
can't). I'll try //lowering// the brightness instead.
  
  
  You could try lowering the contrast. I think we have a shader that does that.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D5245

To: broulik, #vdg, graesslin, #kwin, #plasma
Cc: fredrik, luebking, kvermette, graesslin, plasma-devel, kwin, progwolff, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol

D4963: Add scaling support into BlurEffect::doBlur

[Fredrik Höglund]

fredrik added a comment.


  The blur shader is not designed to blur and scale simultaneously.
  
  The sampled offsets are computed so that the shader samples between pixels, 
and the kernel weights are the sums of the gaussian function for the 
interpolated pixel values at each offset.
  
  This only works when the source and destinations have the same scale.

REVISION DETAIL
  https://phabricator.kde.org/D4963

To: davidedmundson, #plasma
Cc: fredrik, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

[Differential] [Commented On] D4366: WIP: Add screen recorder interface

[Fredrik Höglund]

fredrik added a comment.


  I know this revision has been abandoned, but I wanted to comment on a few 
things for future reference.

INLINE COMMENTS

> screencast.cpp:105
> +
> +QRect geometry = effects->virtualScreenGeometry();
> +

I strongly suggest that you use the damage information from kwin and only 
download the parts of the framebuffer that have actually changed. This can make 
a massive difference in performance.

You can keep a screen sized QImage around and keep it in sync by updating areas 
as they change.

> screencast.cpp:107
> +
> +GLTexture tex(GL_RGBA8, geometry.width(), geometry.height());
> +GLRenderTarget target(tex);

Why do you use an intermediate texture instead of reading directly from the 
framebuffer in the desktop GL case?

You create the texture and blit the framebuffer into it in the GLES case, even 
though you don't use it.

> screencast.cpp:109
> +GLRenderTarget target(tex);
> +target.blitFromFramebuffer(geometry);
> +

There is no need to use a RenderTarget here. Use glCopyTexSubImage2D() instead, 
which copies from the framebuffer to the currently bound texture.

> screencast.cpp:114
> +
> +auto img = QImage(geometry.size(), QImage::Format_RGB888);
> +if (GLPlatform::instance()->isGLES()) {

Don't read back data directly to client memory. Instead create a 
GL_PIXEL_PACK_BUFFER and download the image into it. That way 
glReadPixels()/glGetTexImage() schedules the copy, but doesn't block and wait 
for it to finish. Wait at least one frame before you access the contents of the 
buffer.

> screencast.cpp:116
> +if (GLPlatform::instance()->isGLES()) {
> +glReadPixels(0, 0, img.width(), img.height(), GL_RGB, 
> GL_UNSIGNED_BYTE, (GLvoid*)img.bits());
> +} else {

Never read back data as GL_RGB. Three component formats are not supported in 
hardware, so this involves at least a partial software fallback.

The only format/type combination a GLES implementation is required to support 
is also GL_RGBA/GL_UNSIGNED_BYTE.

I also note that you immediately convert the image to a four-component format 
below.

> screencast.cpp:118
> +} else {
> +glGetTexImage(GL_TEXTURE_2D, 0, GL_RGB, GL_UNSIGNED_BYTE, 
> (GLvoid*)img.bits());
> +}

If you need the image to be in QImage::Format_ARGB32, you should read back the 
data as GL_BGRA/GL_UNSIGNED_INT_8_8_8_8_REV. This is the GL equivalent of 
QImage::Format_ARGB32.

> screencast.cpp:121
> +img = img.convertToFormat(QImage::Format_ARGB32);
> +img = img.mirrored();
> +tex.unbind();

Use GL_MESA_pack_invert and GL_ANGLE_pack_reverse_row_order so the image is 
downloaded in the correct orientation.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D4366

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: fredrik, graesslin, subdiff, plasma-devel, kwin, #kwin, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol

[Differential] [Updated] D4400: Plasma 5.10 "Cascade" Wallpaper

[Fredrik Höglund]

fredrik added a comment.


  In https://phabricator.kde.org/D4400#82337, @kvermette wrote:
  
  > In https://phabricator.kde.org/D4400#82334, @broulik wrote:
  >
  > > Did you try my suggestion regarding optimizing the image for a lossy 
compression algorithm?
  > >
  > > Fredrik made a Khronos KTX (compressed texture) reader for plasma 
wallpapers which could significantly reduce memory consumption for the 
wallpaper, especially for the 4K variant. I think we should give it a try for 
5.10?
  > >
  > > Also, please consider adding a 1920x1200 variant. There were complaints 
that the 5.9 wallpaper didn't come with this size (I also have that resolution 
for my desktop)
  >
  >
  > For some reason this completely slipped my mind; can you give me a quick 
refresher please?
  
  
  OpenGL 4 and ES 3.1 add support for new high quality texture compression 
formats, such as BPTC on desktops, and ASTC on mobile. An uncompressed 4K 
texture consumes 32 MB of video memory, whereas a compressed texture with the 
same resolution only consumes 8 MB.
  
  These formats are not lossless, so a compressed image is not going to be 
identical to the original image. Photographic images tend to compress extremely 
well, to the point where it is difficult to tell the compressed image apart 
from the original image. Vector images with sharp, precise outlines often do 
not compress well, since any compression artifacts that appear along an outline 
are going to be noticeable.
  
  The fredrik/bptc branch in plasma-workspace adds support to the wallpaper 
plugin for loading compressed textures saved as .ktx files.
  
  I have also ported Microsoft's MIT licensed BPTC encoder to OpenGL:
  
  https://cgit.kde.org/scratch/fredrik/bptcencoder.git
  
  This encoder uses compute shaders to encode the image.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D4400

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kvermette, #plasma, #plasma:_design, fredrik
Cc: mart, broulik, andreaska, plasma-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Closed] D3973: wallpapers/image: Use QImageReader in ImageSizeFinder::run()

[Fredrik Höglund]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:ee245780f236: wallpapers/image: Use QImageReader in 
ImageSizeFinder::run() (authored by fredrik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3973?vs=9731=9735

REVISION DETAIL
  https://phabricator.kde.org/D3973

AFFECTED FILES
  wallpapers/image/backgroundlistmodel.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fredrik, #plasma, davidedmundson, broulik
Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas

[Differential] [Request, 4 lines] D3973: wallpapers/image: Use QImageReader in ImageSizeFinder::run()

[Fredrik Höglund]

fredrik created this revision.
fredrik added a reviewer: Plasma.
fredrik set the repository for this revision to R120 Plasma Workspace.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  There is no need to decode the whole image and load it into memory
  when we are only interested in the dimensions.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3973

AFFECTED FILES
  wallpapers/image/backgroundlistmodel.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fredrik, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Commented On] D2484: [Image Wallpaper] Avoid cross-fading which reveals black background behind images

[Fredrik Höglund]

fredrik added a comment.


  The correct way to fix this is to use a custom shader that does approximately 
this:
  
uniform sampler2D texture1;
uniform sampler2D texture2;
uniform float fadeProgress;
varying vec2 texcoord1;
varying vec2 texcoord2;

void main(void) {
vec4 texel1 = texture2D(texture1, texcoord1);
vec4 texel2 = texture2D(texture2, texcoord2);
gl_FragColor = mix(texel1, texel2, fadeProgress);
}
  
  As an added bonus you only render each pixel once, and you don't need 
blending to be enabled, so it's much faster. Especially on tiled renderers.

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D2484

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, mart
Cc: fredrik, mart, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas

Re: Review Request 126131: Don't delete gl texture twice in thumbnail

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126131/#review88705
---


Deleting a texture twice is legal in OpenGL, since unused texture ID's are 
silently ignored by glDeleteTextures(). That doesn't mean it's a good idea to 
do it, since a texture ID can be reused for a different texture after it's been 
passed to glDeleteTextures(). But regardless, it shouldn't crash as long as the 
textures pointer is valid.

- Fredrik Höglund


On Nov. 22, 2015, 1:57 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126131/
> ---
> 
> (Updated Nov. 22, 2015, 1:57 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> The QSGTextures are created with
> 
> window()->createTextureFromId(m_texture, QSize(w,h),
> QuickWindow::TextureOwnsGLTexture));
> 
> this means we don't want to be deleting textures ourselves too, it will
> be deleted when we delete the QSGTexture, which is a scoped pointer
> inside our QSGNode.
> 
> BUG: 355644
> REVIEW:
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/core/windowthumbnail.cpp 
> 2b09657e8ce6bd1cca1acc323d5955b2f1a1efb2 
> 
> Diff: https://git.reviewboard.kde.org/r/126131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 123473: Port mouse theme kcm to QML

[Fredrik Höglund]

 On April 23, 2015, 11:31 a.m., Eike Hein wrote:
   This is more an experiment on how much modules can be closely ported (and 
   in how much time).
  
  What's the benefit to the user of merging this version now?
 
 Marco Martin wrote:
 none.
 not too much pain as well tough.
 all of them have to eventually be ported tough and in order to get done, 
 one has to.. do it
 
 Eike Hein wrote:
  all of them have to eventually be ported tough and in order to get 
 done, one has to.. do it
 
 I'm just not a big fan of putting transitional pain (worse UI from a 
 weaker toolkit) on the user when there's an opportunity to avoid it, I guess 
 ... right now, Qt Quick has worse performance, no keyboard accelerator 
 management, no form layouts, limited widgets, some visual problems, etc. - 
 It's true of course that using it builds greater pressure to get it fixed, 
 but are we *certain* that actively hurting the quality of our releases is the 
 only path available?
 
 Marco Martin wrote:
 bah, right now accelerators and tab focus kinda works in that module..
 still kinda, but again, if the decision is to go in that direction, of 
 which i remeber it was talked about and decided, otherwise I wouldn't have 
 wasted two days on it ;)
 Now, I'm fine if now we decide to not port modules, but most of them 
 kindof have to be redone anyways, and I would prefer reding them once rather 
 than twice.
 
 David Edmundson wrote:
  It's true of course that using it builds greater pressure to get it 
 fixed, but are we certain that actively hurting the quality of our releases 
 is the only path available?
 
 It's not as simple as saying using new stuff /will/ hurt the quality 
 compared to the current state.
 
 This KCM wouldn't use form layouts, or any special widgets that we don't 
 have anyway. Keyboard accelorators and tab keys /should/ work in QQC so by 
 the time we finish with this, I think we can make it just as perfect /and/ 
 progress our QQC integration at the same time.
 
 Also it's not like these KCMs are truly perfect as-is. There are 8 open 
 bugs on the cursor KCM. I'd like to think paying some attention to these KCMs 
 will fix some of them.
 
 I do completely agree with you that users shouldn't be hurt by porting 
 efforts and we should have an absolutely no regressions at all policy before 
 merging, with no excuses about limitations in QQC.
 
 Martin Gräßlin wrote:
 I agree with David that we also should see this as a chance. For example 
 I always wondered why there is this strange preview area on the top, 
 instead of just previewing all cursors in the list directly. With QQC that 
 becomes quite easier and removes the it's probably because it would be a 
 nightmare with delegates.
 
 Marco Martin wrote:
 having all the previews inline could probably be simpler since i could 
 perhaps avoid a custom qquickpainteditem.. however, it would look very 
 crowded i think?
 
 Martin Gräßlin wrote:
 maybe only show them for the selected or on hover?
 
 Marco Martin wrote:
 this is very quick and dirty:
 http://wstaw.org/m/2015/04/24/plasma-desktopzp1576.png
 
 with very big delegates, it almost looks nice :)
 
 Sebastian Kügler wrote:
 I quite like it. Also, it's really not necessary to be able to view 5 or 
 more themes at the same time, this makes comparison already a lot easier.
 
 Marco Martin wrote:
 this is with a better layout:
 http://wstaw.org/m/2015/04/24/plasma-desktopUj1576.png
 there is still one thing that doesn't logically work too much, that is 
 the size combo box, since it depends from the theme selected (not all themes 
 have all the same sizes available) so that may have to be made inline as 
 well, not sure if it would work well tough
 
 Martin Gräßlin wrote:
  so that may have to be made inline as well, not sure if it would work 
 well tough
 
 Given that it already has this Available size, I think it could be a 
 neat idea to morph it into the delegate.
 
 (o) Resolution Dependent
 (o) |Dropdown| available sizes

 For example I always wondered why there is this strange preview area on the 
 top, instead of just previewing all cursors in the list directly. With QQC 
 that becomes quite easier and removes the it's probably because it would be 
 a nightmare with delegates.

* So that the appearance and behavior of the KCM is consistent with that of the 
icon theme KCM.
* So that the listview looks and behaves identically to other listviews.
* To enable the user to get a quick overview of the installed themes without 
having to scroll.
* Each item in the listview provides three key points of data so as to not 
overwhelm the user with information;
  a single large image of the most important cursor, the name of the theme, and 
finally a short description.

Every design decision was make after careful consideration, and after studying 
the user interface

Re: Review Request 121473: Port mouse dataengine

[Fredrik Höglund]

On Dec. 13, 2014, 4:33 p.m., Andrea Scarpino wrote:
  I know this has already been submitted thanks to Bhushan, but I really have 
  to add a -2 here.
  
  This code has clearly not been ported at all!
  
  Did you even test this before you pushed it, or is this a case of It 
  builds, ship it?
  The fact that others had to tell you that it doesn't build suggests that 
  you didn't even try building it.
 
 Andrea Scarpino wrote:
 Hi Fredrik,
 the old review got 2 Ship It in past and then I rebased this to merge 
 it.
 
 I did it with the help on Bhushan via IRC (if you read, please confirm 
 :-), without building it myself.
 
 Should I revert it?

No, but CursorNotificationHandler needs to be ported correctly.

It should inherit QAbstractNativeEventFilter instead of QWidget,
and reimplement nativeEventFilter() instead of x11Event().


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121473/#review71925
---


On Dec. 13, 2014, 12:50 p.m., Andrea Scarpino wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121473/
 ---
 
 (Updated Dec. 13, 2014, 12:50 p.m.)
 
 
 Review request for Plasma.
 
 
 Repository: plasma-workspace
 
 
 Description
 ---
 
 Rebased https://git.reviewboard.kde.org/r/114260/
 
 
 Diffs
 -
 
   dataengines/mouse/mouseengine.h d55565dc3875b794dce14054d9a8b77cef9b2347 
   dataengines/mouse/mouseengine.cpp f1c898150e04b44dad1c8a0fc757c1ffdf94941b 
   dataengines/CMakeLists.txt cae926b550cdd69aa0194e19bc4818c597e174c0 
   dataengines/mouse/CMakeLists.txt 29cab878bd1fb514d7ac025134fbfb58a5a1376e 
   dataengines/mouse/cursornotificationhandler.h 
 3b571f8f5ffe6db4efeef7827acf003911bcd5dc 
   dataengines/mouse/cursornotificationhandler.cpp 
 cf1c3fbe91a9afee5090d167dd6cd51637df04d4 
 
 Diff: https://git.reviewboard.kde.org/r/121473/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andrea Scarpino
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 121473: Port mouse dataengine

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121473/#review71925
---



dataengines/mouse/cursornotificationhandler.cpp
https://git.reviewboard.kde.org/r/121473/#comment50146

QWidget is not an X window in Qt 5.



dataengines/mouse/cursornotificationhandler.cpp
https://git.reviewboard.kde.org/r/121473/#comment50145

The virtual function reimplemented here doesn't even exist in Qt 5.
Qt 5 does not use XEvents.


I know this has already been submitted thanks to Bhushan, but I really have to 
add a -2 here.

This code has clearly not been ported at all!

Did you even test this before you pushed it, or is this a case of It builds, 
ship it?
The fact that others had to tell you that it doesn't build suggests that you 
didn't even try building it.

- Fredrik Höglund


On Dec. 13, 2014, 12:50 p.m., Andrea Scarpino wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121473/
 ---
 
 (Updated Dec. 13, 2014, 12:50 p.m.)
 
 
 Review request for Plasma.
 
 
 Repository: plasma-workspace
 
 
 Description
 ---
 
 Rebased https://git.reviewboard.kde.org/r/114260/
 
 
 Diffs
 -
 
   dataengines/mouse/mouseengine.h d55565dc3875b794dce14054d9a8b77cef9b2347 
   dataengines/mouse/mouseengine.cpp f1c898150e04b44dad1c8a0fc757c1ffdf94941b 
   dataengines/CMakeLists.txt cae926b550cdd69aa0194e19bc4818c597e174c0 
   dataengines/mouse/CMakeLists.txt 29cab878bd1fb514d7ac025134fbfb58a5a1376e 
   dataengines/mouse/cursornotificationhandler.h 
 3b571f8f5ffe6db4efeef7827acf003911bcd5dc 
   dataengines/mouse/cursornotificationhandler.cpp 
 cf1c3fbe91a9afee5090d167dd6cd51637df04d4 
 
 Diff: https://git.reviewboard.kde.org/r/121473/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andrea Scarpino
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 119913: Force creation of a OpenGL core context

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119913/#review65165
---


This code has no effect. The profile is ignored unless you request version 3.2 
or higher. Furthermore, the renderable type must be set to OpenGL.

- Fredrik Höglund


On Aug. 23, 2014, 3:16 p.m., Alexander Richardson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119913/
 ---
 
 (Updated Aug. 23, 2014, 3:16 p.m.)
 
 
 Review request for Plasma.
 
 
 Repository: plasma-workspace
 
 
 Description
 ---
 
 This was suggested by Martin Gräßlin in review 119524 and also fixes the
 build with Qt 5.4, since QQuickWindow::setDefaultFormat no longer exists
 
 REVIEW: 119913
 
 
 Diffs
 -
 
   krunner/main.cpp 4d8cc31ff4d78c2daa0f9a6af247b3bd1c317ffa 
 
 Diff: https://git.reviewboard.kde.org/r/119913/diff/
 
 
 Testing
 ---
 
 compiles
 
 
 Thanks,
 
 Alexander Richardson
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Minutes Monday Plasma hangout

[Fredrik Höglund]

On Wednesday 19 February 2014, David Edmundson wrote:
 On Mon, Feb 17, 2014 at 1:26 PM, Sebastian Kügler se...@kde.org wrote:
  Plasma Meeting February, 17th, 2014
 
  Present: Alex Fiestas, David Edmundson, Giorgos Tsiapaliokas, Marco Martin,
  Martin Grässlin, Martin Klapetek, Jens Reuterberg, Antonis Tsiapaliokas,
  Sebastian Kügler, Vishesh Handa,
 
 
  Alex:
  - finished wallet work (it's secure now!)
  - works on kdeinit now (benchmarking and profiling to make it lighter)
  - researches overlap with systemd, where can we use systemd?
 
  David:
  - started notes plasmoid (davidedmundson/notes)
  - debugging proxymodel, fixed problem
  - fixed toolbox problems
  - various improvements in tooltip
  - other bugfixes
  - profiling: looking at memory consumption of Plasma
 
 I had a look at the code for QSGTexture, it does indeed store a cache
 of the texture as a QImage. .
 
 Right now we do two paints SVG-QPixmap (the cache) and
 QPixmap-QImage (via QQuickPaintedItem).
 
 This second paint is very problematic for 3 reasons:
 1) it's another paint!
 2) we store the image twice
 3) we are no longer implicitly sharing the same image when we do a
 paint across images as opposed to copying a QImage.
 
 I have just managed to make Plasma::SvgItem provide QSGNodes rather
 than inheriting from QQuickPaintedItem.
 
 This is half way there, the next step is to port most of
 Plasma::Theme/Plasma::SVG to use QImage instead of QPixmap throughout
 (including the cache) so that we can avoid the deep copies and the
 secondary QPainting. This will mean breaking the public API(!), but it
 will be totally worth it.

It's worth noting that QPixmap::toImage() is essentially free with
the raster graphics system, as long as nothing causes the image
to detach.

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KSelectionWatcher / KWindowSystem::compositingChanged signal

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107983/#review24751
---


Because the XFixesSelectSelectionInput() call specifies that the event should 
be delivered to winId(). KSelectionOwner does not send XFixes events; they are 
generated by the X server.


- Fredrik Höglund


On Jan. 5, 2013, 2:38 p.m., Thomas Lübking wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/107983/
 ---
 
 (Updated Jan. 5, 2013, 2:38 p.m.)
 
 
 Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, Marco Martin, 
 Martin Gräßlin, and Fredrik Höglund.
 
 
 Description
 ---
 
 It works fine here (tested so far KWindowSystem signal, KSelectionWatcher 
 only with kwin) with kwin (shift+alt+f12), xcompmgr, compiz  metacity -c 
 and e17.
 Didn't try xfce nor mutter.
 
 Technically:
 I do not at all understand why KWindowSystem is *not* watching the root 
 window - KSelectionOwner for one is sending events to the root and this also 
 seems the case for all other WMs (at least everything now starts to cause the 
 signal to be emitted)
 
 The KSelectionWatcher failure seems to be kwin specific (wrote me a cleaner 
 testcase), there'll be some X11 event processing on top that eats away the 
 client messages.
 So this one can be scratched from the patch, the KWindowSystem issue remains.
 
 
 This addresses bug 179042.
 http://bugs.kde.org/show_bug.cgi?id=179042
 
 
 Diffs
 -
 
   kdeui/windowmanagement/kwindowsystem_x11.cpp f9b3cc1 
 
 Diff: http://git.reviewboard.kde.org/r/107983/diff/
 
 
 Testing
 ---
 
 see summary
 
 
 Thanks,
 
 Thomas Lübking
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: plasma2 and ToolTipManager

[Fredrik Höglund]

On Monday 01 October 2012, Martin Gräßlin wrote:
 On Monday 01 October 2012 15:34:54 Aaron J. Seigo wrote:
  On Monday, October 1, 2012 15:15:23 Martin Gräßlin wrote:
   Am 01.10.2012 14:46, schrieb Aaron J. Seigo:
the GL texture would be generated and updated by the window manager
but used b
other applications (e.g.the desktop shell). how to address such
textures is
platform specific (windows, mac, x11, etc) but it is a broadly
available
functionality and one _we_ only need to care about on a very select #
of
platforms.
   
   sharing OpenGL textures for the windows is an absolute no-go from the
   security point of view in Wayland
  
  in wayland, isn't the desktop shell also a secured (as in: controlled and
  identified) process?
 If it gets started by the session control (or the compositor) probably yes.

The desktop shell is always started by the compositor or it won't
be allowed to bind the desktop shell interface.

 But that doesn't mean that I would like to have texture sharing ;-) I just 
 wrote that quickly down while reading the mails during work and security is 
 for me always the strongest argument.
 
 There are more things which speak against the approach of sharing window 
 textures. I do not want to go into detail but I'm strictly against this 
 approach and would not add it to KWin due to the way how windows are rendered 
 to the screen (note: it's five textures not one). I hope that you trust my 
 judgement in that regard that sharing the window texture from KWin is 
 extremely difficult, in fact so difficult that I suppose a completely 
 different approach which is hardly from trivial either.

KWin would not share the actual texture(s). There is no mechanism that
would allow you to do that even if you wanted to. Instead it would render
the window into a pixmap, and make the pixmap handle known to plasma.
Plasma can then texture directly from the pixmap if it uses OpenGL,
or render it by other means if doesn't.

I don't foresee that this would add much complexity to the thumbnail
effect.

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: IRC meeting summary

[Fredrik Höglund]

On Wednesday 12 September 2012, Ivan Čukić wrote:
  even tough is materal way post 4.10, if there is something really needed for
  plasma that is still not present in the protocol, we should speak up asap
 
 Will not pretend I understand the interface at all, but somebody mentioned 
 there will be no window ids. How will we know which window is the focused and 
 similar stuff?

The compositor will provide a special interface to the desktop shell that
exposes this information.  Regular clients are not allowed to know anything
about windows that belong to other clients.

This reminds me of something I forgot to mention in the IRC meeting.
The D-BUS bindings in kwin and plasma effectively provide a way to
circumvent the security features in wayland.  This is something that we
need to think about going forward.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Add Tilt-Wheel, XButton1, and XButton2 support for Kickoff GUI

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104406/#review11881
---

Ship it!


Ship It!

- Fredrik Höglund


On March 26, 2012, 5:10 p.m., Richard Stockton wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104406/
 ---
 
 (Updated March 26, 2012, 5:10 p.m.)
 
 
 Review request for Plasma, Kevin Ottens, Aaron J. Seigo, and Martin Gräßlin.
 
 
 Description
 ---
 
 Workaround removal of the Back Button (in 4.7): In the
 Applications GUI, users with tilt-wheel mice can use Tilt-Left
 as a Back Button equivalent. And, if the focused item is a
 Parent for other items, Tilt-Right will move to the right,
 expanding the Children.
 
 In addition: For gamer mice with thumb buttons, XButton1
 (AKA the back button) is made to work just like the Tilt-Left
 Wheel Event; and XButton2 is made to work just like Tilt-Right.
 
 
 This addresses bug 289519.
 http://bugs.kde.org/show_bug.cgi?id=289519
 
 
 Diffs
 -
 
   plasma/desktop/applets/kickoff/ui/flipscrollview.h bf12b9c 
   plasma/desktop/applets/kickoff/ui/flipscrollview.cpp 98bc0bd 
 
 Diff: http://git.reviewboard.kde.org/r/104406/diff/
 
 
 Testing
 ---
 
 Compiled, Built and Run. (Although another function, which I did not touch, 
 provokes two unused variable warnings).
 My desktop is otherwise 4.8.1, and I am using this code in production. 
 New buttons work; Tilt wheel works; I have seen no regressions.
 
 
 Thanks,
 
 Richard Stockton
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Scroll automatically when touching the border with the rubberband

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104396/#review11809
---

Ship it!


Ship It!

- Fredrik Höglund


On March 24, 2012, 12:42 p.m., Ignat Semenov wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104396/
 ---
 
 (Updated March 24, 2012, 12:42 p.m.)
 
 
 Review request for Plasma, Aaron J. Seigo and Fredrik Höglund.
 
 
 Description
 ---
 
 Wehn dragging a rubberband over an icon view with a scrollbar, scroll the 
 view automatically on touching the border with the mouse during drag.
 
 I've had to mod stopAutoScrolling() and call it in IconView::wheelEvent() and 
 AbstractItemView::scrollBarActionTriggered() to allow the user to stop the 
 automatic scrolling on those events.
 
 
 This addresses bug 189350.
 http://bugs.kde.org/show_bug.cgi?id=189350
 
 
 Diffs
 -
 
   plasma/applets/folderview/abstractitemview.cpp 3debb70 
   plasma/applets/folderview/iconview.h 0bd6dc0 
   plasma/applets/folderview/iconview.cpp 36e640b 
 
 Diff: http://git.reviewboard.kde.org/r/104396/diff/
 
 
 Testing
 ---
 
 Tested, works.
 
 Maybe we should also stop automatic scrolling if the user releases the LMB 
 while scrollling automatically as a result of dragging the rubberband over 
 the view border? Fredrik, what do you think?
 
 
 Thanks,
 
 Ignat Semenov
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: do not crash when passing a url in the ctor e.g. when creating a folderview by dropping a folder onto the desktop

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104311/#review11504
---

Ship it!


Ship It!

- Fredrik Höglund


On March 17, 2012, 9:52 a.m., Ignat Semenov wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104311/
 ---
 
 (Updated March 17, 2012, 9:52 a.m.)
 
 
 Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund.
 
 
 Description
 ---
 
 Do not call setUrl in the ctor since m_dirLister does not exist yet. Instead, 
 assign m_url in the ctor and call setUrl later in init().
 
 
 Diffs
 -
 
   plasma/applets/folderview/folderview.cpp a94ce87 
 
 Diff: http://git.reviewboard.kde.org/r/104311/diff/
 
 
 Testing
 ---
 
 Tested, works.
 
 Would be nice to give the code a static analyzer run, if there was a decent 
 static analyzer available in Linux. Anybody have relevant experience?
 
 
 Thanks,
 
 Ignat Semenov
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Save scrollbar position on plasma exit

[Fredrik Höglund]

 On March 13, 2012, 1:12 p.m., Marco Martin wrote:
  looks good, a thing i would like to be tested is when the saved position is 
  invalid, like either negative or an enormous value.
  
  this shouldn't break it (is even quite probable a value not being valid 
  anymore because there are less files than the previous session)

I think it would be a good idea to save the modification time for the folder 
and use that to check if the saved scrollbar value is likely to be invalid. If 
the user is able to scroll the view while the layout is in progress, this 
should also abort restoring the position.

Also, I'm wondering if we really need to save the position separately for the 
iconview and the listview, or if we should use the same key.

Otherwise the patch looks good to me.


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104258/#review11354
---


On March 13, 2012, 1:51 p.m., Ignat Semenov wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104258/
 ---
 
 (Updated March 13, 2012, 1:51 p.m.)
 
 
 Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund.
 
 
 Description
 ---
 
 This patch implements scrolbar position saving on plasma exit. The change is 
 fairly trivial, however, due to the fact that the view is not populated and 
 layouted immediately simply scrolling to the desired position on creating the 
 view does not work. Instead a signal is emitted on finishing the item layout, 
 when the view has a valid size and the scrollbar has a valid range. The 
 signal is connected to a slot which scrolls the view to the desired position 
 and then disconnects the signal. For the user, a public function in 
 AbstractItemView is introduced, which performs the connection.
 
 The only problem is that ListView turned out not to have any layout method. 
 It just paints the items one by one, calculating their position on the fly, 
 so I put the signal at the end of updateScrollbar to ensure the scrollbar 
 range is valid. Maybe it should go into the if (max0) branch?
 
 
 This addresses bug 261139.
 http://bugs.kde.org/show_bug.cgi?id=261139
 
 
 Diffs
 -
 
   plasma/applets/folderview/abstractitemview.h aa68b90 
   plasma/applets/folderview/abstractitemview.cpp 3debb70 
   plasma/applets/folderview/folderview.h 4e441eb 
   plasma/applets/folderview/folderview.cpp a94ce87 
   plasma/applets/folderview/iconview.cpp 5c4e086 
   plasma/applets/folderview/listview.cpp b17e7c4 
 
 Diff: http://git.reviewboard.kde.org/r/104258/diff/
 
 
 Testing
 ---
 
 Tested both the icon view and the list view, works fine.
 
 
 Thanks,
 
 Ignat Semenov
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Take sorting order into account in ProxyModel::lessThan() in order to give folders precedence regardless of the sort order used

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103884/#review10404
---

Ship it!


Ship It!

- Fredrik Höglund


On Feb. 7, 2012, 7:40 p.m., Ignat Semenov wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/103884/
 ---
 
 (Updated Feb. 7, 2012, 7:40 p.m.)
 
 
 Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund.
 
 
 Description
 ---
 
 Currently, after the sort order patch 
 https://git.reviewboard.kde.org/r/103860/, folders are moved to the bottom of 
 the sorting list with the sorting order set to Descending. This obviously 
 is not what the author wanted to do with the option Folders first, so this 
 patch tries to place folders on top of the sorting list, even if they are 
 still sorted according to the selected sorting order (which, in my opinion, 
 is perfectly fine).
 
 
 Diffs
 -
 
   plasma/applets/folderview/proxymodel.cpp ed28416 
 
 Diff: http://git.reviewboard.kde.org/r/103884/diff/diff
 
 
 Testing
 ---
 
 Tested, builds and works as described above.
 
 
 Thanks,
 
 Ignat Semenov
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Less repainting on mousePressEvent(), moseReleaseEvent() and mouseDoubleClickEvent() in FolderView::IconView

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103822/#review10360
---

Ship it!


Aside from some minor nitpicks, it looks good.


plasma/applets/folderview/iconview.cpp
http://git.reviewboard.kde.org/r/103822/#comment8527

Make these const. There is also a whitespace error on this line.



plasma/applets/folderview/iconview.cpp
http://git.reviewboard.kde.org/r/103822/#comment8526

Change the name of this variable to 'rect'.


- Fredrik Höglund


On Jan. 29, 2012, 3:54 p.m., Ignat Semenov wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/103822/
 ---
 
 (Updated Jan. 29, 2012, 3:54 p.m.)
 
 
 Review request for Plasma, Aaron J. Seigo and Marco Martin.
 
 
 Description
 ---
 
 This patch aims to save some repaints in FolderView::IconView on the various 
 mouseEvent()'s by choosing what to repaint in a bit smarter way.
 
 
 Diffs
 -
 
   plasma/applets/folderview/iconview.h 66ccb98 
   plasma/applets/folderview/iconview.cpp 5b0cd98 
 
 Diff: http://git.reviewboard.kde.org/r/103822/diff/diff
 
 
 Testing
 ---
 
 Testing done against master, seems to behave indentically.
 
 
 Thanks,
 
 Ignat Semenov
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: folderview broken

[Fredrik Höglund]

On Wednesday 21 December 2011, Aaron J. Seigo wrote:
 On Monday, December 19, 2011 09:25:41 sujith h wrote: 
  Are you trying to access remote site or local host? I had not done anything
  for
  local host. If you are trying to access the remote host, can you please
  share the URL for that, so that I can also try.
 
 did some looking into this today. the problem was visible when the URL is 
 considered non-local and is a containment.
 
 since the change made had init() return early, code further down that did 
 things like set up the icon view when it is a containment was not run.
 
 making it worse, desktop:/ is a non-local file url (according to KUrl anyways 
 :) so if that is what it was pointing at in the config file, that would be 
 enough to trigger the problem.

Aaron, thanks for fixing this. Just a note that in this case
KProtocolInfo::protocolClass() should be used to determine if the URL
is local or not, since this also affects trash, nepomuk and so forth.

KUrl::isLocalFile()/toLocalFile() should only be used to check if the URL
can be accessed without using KIO.

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: compositingActive not efficient on XRandR events

[Fredrik Höglund]

On Monday 18 July 2011, Aaron J. Seigo wrote:
 On Saturday, July 16, 2011 15:36:19 Martin Gräßlin wrote:
  The bug is much simpler - Plasma just simply fails to recognize that a
  compositing manager is active. I can see this each time I restart kwin
  (which is considerable often). In order to get translucent panels back, I
  have to kquitapp plasma-desktop, plasma-desktop.
 
 Plasma::Theme uses a KSelectionWatcher which watches _NET_WM_CM_S# where # is 
 the number of the default screen.
 
 i can imagine a few things going wrong with this:
 
 * the default screen # changes or even goes away completely; that could 
 render 
 the selection manager useless. why is the CM atom per screen again? *sigh*

Screen here is about traditional multi-head where you can have different
window managers / compositing managers managing each screen.

 * a race condition as Alex outlined. if kwin is indeed responding to each 
 xrandr even with a change in the CM, that seems like a perfect candidate for 
 event compression if at all possible: don't tell the world outside that 
 things 
 have changed until the events have stopped coming in. the timeout for this 
 shouldn't need to be long at all, so the user shouldn't see a big change at 
 all
 
 * KSelectionWatcher itself and/or kwin's setting of the atom could be broken. 
 in times past we've had isses where the KSelectionWatcher object simply did 
 not emit any signals at all when kwin changed compositing.

I thought this bug was fixed, because I haven't seen it in a long time.
But I realized that I still have a patch applied that I created a couple
of years ago.

It adds a compositingChanged() signal to KWindowSystem and makes
Plasma::Theme use it. It also uses Xfixes instead of KSelectionWatcher
to monitor the selection.

I never committed the patch because Lubos didn't like the idea. IIRC the
reason was that the signal would be emitted before the compositing
plugins are initialized in kwin.

I have attached the diff in case it fixes the problem for those who are
still seeing it.

Regards,
Fredrik

diff --git a/kdeui/windowmanagement/kwindowsystem.h b/kdeui/windowmanagement/kwindowsystem.h
index dba1da4..6b79cb0 100644
--- a/kdeui/windowmanagement/kwindowsystem.h
+++ b/kdeui/windowmanagement/kwindowsystem.h
@@ -621,6 +621,12 @@ Q_SIGNALS:
  */
 void showingDesktopChanged( bool showing );
 
+/**
+ * Compositing was enabled or disabled.
+ * @since 4.4
+ */
+void compositingChanged( bool enabled );
+
 protected:
 virtual void connectNotify( const char* signal );
 
diff --git a/kdeui/windowmanagement/kwindowsystem_x11.cpp b/kdeui/windowmanagement/kwindowsystem_x11.cpp
index 51b8e56..741393c 100644
--- a/kdeui/windowmanagement/kwindowsystem_x11.cpp
+++ b/kdeui/windowmanagement/kwindowsystem_x11.cpp
@@ -36,6 +36,12 @@
 #include QtGui/QX11Info
 #include X11/Xatom.h
 
+#include config.h
+
+#ifdef HAVE_XFIXES
+#include X11/extensions/Xfixes.h
+#endif
+
 class KWindowSystemStaticContainer {
 public:
 KWindowSystemStaticContainer() : d(0) {}
@@ -46,6 +52,8 @@ public:
 
 K_GLOBAL_STATIC(KWindowSystemStaticContainer, g_kwmInstanceContainer)
 
+static Atom net_wm_cm;
+static void create_atoms( Display* dpy = QX11Info::display() );
 
 static unsigned long windows_properties[ 2 ] = { NET::ClientList | NET::ClientListStacking |
  NET::Supported |
@@ -92,7 +100,10 @@ public:
 QListStrutData strutWindows;
 QListWId possibleStrutWindows;
 bool strutSignalConnected;
+bool compositingEnabled;
+bool haveXfixes;
 int what;
+int xfixesEventBase;
 bool mapViewport();
 
 void addClient(Window);
@@ -110,10 +121,23 @@ KWindowSystemPrivate::KWindowSystemPrivate(int _what)
_what = KWindowSystem::INFO_WINDOWS ? windows_properties : desktop_properties,
2, -1, false ),
   strutSignalConnected( false ),
+  haveXfixes( false ),
   what( _what )
 {
 KSystemEventFilter::installEventFilter(this);
 (void ) qApp-desktop(); //trigger desktop widget creation to select root window events
+
+#ifdef HAVE_XFIXES
+int errorBase;
+if ((haveXfixes = XFixesQueryExtension(QX11Info::display(), xfixesEventBase, errorBase))) {
+create_atoms();
+XFixesSelectSelectionInput(QX11Info::display(), winId(), net_wm_cm,
+   XFixesSetSelectionOwnerNotifyMask |
+   XFixesSelectionWindowDestroyNotifyMask |
+   XFixesSelectionClientCloseNotifyMask);
+compositingEnabled = XGetSelectionOwner(QX11Info::display(), net_wm_cm) != None;
+}
+#endif
 }
 
 // not virtual, but it's called directly only from init()
@@ -127,6 +151,18 @@ bool KWindowSystemPrivate::x11Event( XEvent * ev )
 {
 KWindowSystem* s_q = KWindowSystem::self();
 
+#ifdef HAVE_XFIXES
+if ( ev-type == xfixesEventBase + XFixesSelectionNotify  ev-xany.window == winId() ) {
+  

Re: the next step on the desktop

[Fredrik Höglund]

On Monday 31 January 2011, Martin Gräßlin wrote:
 - Ursprüngliche Mitteilung -
  A Segunda, 31 de Janeiro de 2011 13:00:50 Marco Martin você escreveu:
   provocation: remove the maximize button by default? ;) (as the netbook
   doesn't have minimize)
  
  will talk to mgrslin this afternoon but somthing like a optimal size
  buton   instead of max... if you clik it again it goes max now the
  apps would need   to provide info to kwin about it... this comes in line
  with the other crazy   ideas we benn cooking. worth trying IMO
 at UDS I talked with some desktop experience devs about that. What we came up 
 with is abusing the (unused) maximum size hint and making the maximize button 
 tri-state. First click optimized, second click maximized, third click back to 
 normal. As Ubuntu is also interested there's the chance of getting some kind 
 of standard (but honestly I doubt that gnome would be collaborative in any 
 way if it involves both kde and canonical).

The maximum size hint is not unused. QWidget exposes a property for
setting it, and KWin respects it. In addition the only way to make a window
fixed-size is to set the minimum and maximum size hints to the same value.

This is specified not just by the ICCCM, but also by the EWMH.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Folderview : label when empty folder

[Fredrik Höglund]

 On 2010-07-08 08:31:35, Beat Wolf wrote:
  Looks good to me, and trunk is open again.
 
 Iamluc wrote:
 Thanks you for the review.
 
 I don't have an SVN account yet (and I think it's too early to ask one). 
 So could you please commit the patch for me ?
 
 Simon Stiphanos wrote:
 Hello IamLuc and Beat Wolf,
 
 I reported the original bug and would like to point out how to better fix 
 it than this patch currently does. The Plasma::Label is extremely large for 
 what is being reported. It should be much smaller to encase just enough for 
 the text because there is no reason to waste such a large amount of real 
 estate.
 
 Do you agree?
 
 Beat Wolf wrote:
 i wait for now with commiting
 
 Iamluc wrote:
 Hi Simon,
 
 Maybe the popup is too big for an empty folder, but we need empty space 
 in case we want to create a new file (ie, right-click  new) or paste 
 files/folders.
 
 For the moment, all the popups have the same size. More generally than 
 for an empty folder, I think what you want is to automatically resize the 
 popup according to its content (If there is only 1 file, the popup must be 
 smaller than if there are 10 files).
 Am I correct ? If yes, I think we need advices/comments about this 
 behaviour with more experimented plasma developpers (It's only my first 
 patch !)
 
 (Sorry for my poor english, I hope you will understand  :-/)

 
 Simon Stiphanos wrote:
 Thanks for the reply IamLuc, but that's not exactly what I mean. Is it at 
 all possible to resize just this element? (And your English is still very 
 readable :)

The IconView already has its own code for displaying messages inside the view 
(see IconView::paintMessage()).
This is used for reporting errors to user when listing the folder.

I would also consider this a bugfix and not a new feature.


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4519/#review6422
---


On 2010-07-03 17:11:02, Iamluc wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/4519/
 ---
 
 (Updated 2010-07-03 17:11:02)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 Hello,
 
 This patch add a Plasma::Label with text This folder is empty. in 
 Folderview's Popup when the folder is empty.
 
 The related bug number in kde bugzilla is 201542.
 
 Luc
 
 
 This addresses bug 201542.
 https://bugs.kde.org/show_bug.cgi?id=201542
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 1145373 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1145373 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/popupview.h 1145373 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/popupview.cpp 1145373 
 
 Diff: http://reviewboard.kde.org/r/4519/diff
 
 
 Testing
 ---
 
 Delete/add files several times. The label disappears/appears correctly.
 
 
 Screenshots
 ---
 
 folderview empty folder
   http://reviewboard.kde.org/r/4519/s/444/
 
 
 Thanks,
 
 Iamluc
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Folderview : label when empty folder

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4519/#review6434
---


I wouldn't worry about the size just now, it can be done in a separate commit. 
I actually have a patch that adjusts the size based on the number of items, but 
I couldn't decide if I liked it or not.

I think showing the information icon next to the text might be a bit too much 
though.


/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/4519/#comment6166

Does it still work if this is changed to
if (m_model-rowCount() == 0  !listingInProgress())?

It would simplify the rest of the code a bit if it does.
It should probably also be 'else if', so two messages aren't overlaid.



- Fredrik


On 2010-07-08 19:22:01, Iamluc wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/4519/
 ---
 
 (Updated 2010-07-08 19:22:01)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 Hello,
 
 This patch add a Plasma::Label with text This folder is empty. in 
 Folderview's Popup when the folder is empty.
 
 The related bug number in kde bugzilla is 201542.
 
 Luc
 
 
 This addresses bug 201542.
 https://bugs.kde.org/show_bug.cgi?id=201542
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 1145373 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1145373 
 
 Diff: http://reviewboard.kde.org/r/4519/diff
 
 
 Testing
 ---
 
 Delete/add files several times. The label disappears/appears correctly.
 
 
 Screenshots
 ---
 
 folderview empty folder
   http://reviewboard.kde.org/r/4519/s/444/
 version 2
   http://reviewboard.kde.org/r/4519/s/455/
 
 
 Thanks,
 
 Iamluc
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: system tray work

[Fredrik Höglund]

On Friday 07 May 2010, Aurélien Gâteau wrote:
 On 02/05/2010 23:36, Fredrik Höglund wrote:
  Anyway, I've attached the current version of the Klipper patch.
  Aside from the keyboard shortcut there's also the issue that the
  dbus menu isn't always updated when the clipboard history changes.
  Sometimes it is, sometimes it isn't.
  
  I'm hoping Aurélien has some idea about that.
 
 I just had a look at it, and could not reproduce it :/. Do you know of a
 way to reliably reproduce it?

Yes, select some text and left-click the klipper icon. Then click outside
the menu so it goes away, and right-click the icon. You should see that
the dbus menu hasn't been updated with the new selection.
And from this point on the dbus menu will remain as it is, while the
left-click menu continues to be updated with each new selection. 

 About the patch: I noticed that left-click shows the menu directly
 instead of going through dbusmenu, which is a bit inconsistent (even if
 it's nice  for testing!). I tried to improve it by replacing the code in
 tray.cpp like this:

snip
 
 (Calling setAssociatedWidget() on the menu is a little-known feature of
 KSNI which causes the left-click to trigger the menu)

Yes, that looks good. I wasn't aware of that feature actually.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: system tray work

[Fredrik Höglund]

On Friday 07 May 2010, Aaron J. Seigo wrote:
 On May 7, 2010, Aurélien Gâteau wrote:
  But it's not enough because KStatusNotifierItem shows the menu itself
  instead of sending a menu request over DBus. Which is understandable
  because there is currently no way to send a menu request. This would be
  a good addition in my opinion, what do you think about this?
 
 i'm not exactly sure what you mean by sending a menu request ... at what 
 point (besides the context menu) does KSNI decide to show a menu? (and in the 
 case of the context menu, we do send a request)

Klipper only provides a context menu, so it wants it to be shown regardless
of whether it's the primary or secondary action that's triggered.

The alternative is to do nothing when the icon is left-clicked.
It is a consistency problem though that some icons will show the menu
on left-click while others will only show it on right-click.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: system tray work

[Fredrik Höglund]

On Thursday 29 April 2010, Aaron J. Seigo wrote:
 On April 29, 2010, Marco Martin wrote:
  i think i like more the second option.
  the only problem is that the ui for it would be rather clunky (there would
  be two distinct shortcut configurations in 2 different places that do
  almost the same thing)
 
 yes, a little clunky. perhaps room for improvement in the future. bonus 
 points 
 for working with any entry in the system tray though ;)
 
 which reminds me: someday we really ought to implement some keyboard 
 navigation :)

I like this option as well. Keyboard shortcuts are important for
accessability, so this is something that really should work with all
icons and not just with klipper.

Anyway, I've attached the current version of the Klipper patch.
Aside from the keyboard shortcut there's also the issue that the
dbus menu isn't always updated when the clipboard history changes.
Sometimes it is, sometimes it isn't.

I'm hoping Aurélien has some idea about that.

Regards,
Fredrik

Index: klipper.cpp
===
--- klipper.cpp	(revision 1121129)
+++ klipper.cpp	(working copy)
@@ -356,27 +356,17 @@
 Q_ASSERT( menu != 0L );
 
 QSize size = menu-sizeHint(); // geometry is not valid until it's shown
-if (m_bPopupAtMouse) {
-QPoint g = QCursor::pos();
-if ( size.height()  g.y() )
-menu-popup(QPoint( g.x(), g.y() - size.height()));
-else
-menu-popup(QPoint(g.x(), g.y()));
-} else {
-if( KSystemTrayIcon* tray = dynamic_cast KSystemTrayIcon* ( parent())) {
-QRect g = tray-geometry();
-QRect screen = KGlobalSettings::desktopGeometry(g.center());
+QPoint pos;
 
-if ( g.x()-screen.x()  screen.width()/2 
- g.y()-screen.y() + size.height()  screen.height() )
-menu-popup(QPoint( g.x(), g.y() - size.height()));
-else
-menu-popup(QPoint( g.x() + g.width(), g.y() + g.height()));
-} else
-abort();
+if ( m_bPopupAtMouse )
+pos = QCursor::pos();
+else
+pos = m_trayPos;
 
-//  menu-exec(mapToGlobal(QPoint( width()/2, height()/2 )));
-}
+if ( size.height()  pos.y() )
+pos.ry() -= size.height();
+
+menu-popup(pos);
 }
 
 bool Klipper::loadHistory() {
@@ -581,6 +571,11 @@
 showPopupMenu( popup );
 }
 
+void Klipper::slotPopupMenu(bool active, const QPoint pos) {
+Q_UNUSED(active)
+m_trayPos = pos;
+slotPopupMenu();
+}
 
 void Klipper::slotRepeatAction()
 {
Index: tray.cpp
===
--- tray.cpp	(revision 1121129)
+++ tray.cpp	(working copy)
@@ -28,16 +28,23 @@
 
 #include klipper.h
 #include history.h
-#include KPassivePopup
+#include klipperpopup.h
 
+#include KNotification
+
 KlipperTray::KlipperTray()
-: KSystemTrayIcon( klipper )
+: KStatusNotifierItem()
 {
 m_klipper = new Klipper( this, KGlobal::config());
-setContextMenu( NULL );
-show();
-connect( this, SIGNAL( activated( QSystemTrayIcon::ActivationReason )), m_klipper,
-SLOT( slotPopupMenu()));
+setTitle( i18n( Klipper ) );
+setIconByName( klipper );
+setToolTip( klipper, i18n( Clipboard Contents ), i18n( Clipboard is empty ) );
+setCategory( SystemServices );
+setStatus( Active );
+setStandardActionsEnabled( false );
+setContextMenu( m_klipper-history()-popup() );
+connect( this, SIGNAL( activateRequested( bool, QPoint )), m_klipper,
+SLOT( slotPopupMenu( bool, QPoint)));
 connect( m_klipper-history(), SIGNAL(changed()), SLOT(slotSetToolTipFromHistory()));
 slotSetToolTipFromHistory();
 connect( m_klipper, SIGNAL(passivePopup(QString,QString)), SLOT(passive_popup(QString,QString)));
@@ -46,17 +53,22 @@
 void KlipperTray::slotSetToolTipFromHistory()
 {
 if (m_klipper-history()-empty()) {
-  setToolTip( i18n(Clipboard is empty));
+  setToolTipSubTitle( i18n(Clipboard is empty));
 } else {
   const HistoryItem* top = m_klipper-history()-first();
-  setToolTip(top-text());
+  setToolTipSubTitle(top-text());
 }
-
 }
 
 void KlipperTray::passive_popup(const QString caption, const QString text)
 {
-KPassivePopup::message(KPassivePopup::Boxed, caption, text, icon().pixmap(QSize(16,16)), this);
+if (m_notification) {
+m_notification-setTitle(caption);
+m_notification-setText(text);
+} else {
+m_notification = KNotification::event(KNotification::Notification, caption, text,
+  KIcon(klipper).pixmap(QSize(16, 16)));
+}
 }
 
 #include tray.moc
Index: klipper.h
===
--- klipper.h	(revision 1121129)
+++ klipper.h	(working copy)
@@ -156,6 +156,7 @@
 
 public Q_SLOTS:
 void slotPopupMenu();
+void slotPopupMenu(bool active, const QPoint pos);
 

Re: Review Request: Add simple Type-and-Select feature to Folderview applet

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2659/#review3783
---


The code looks good, but I'm slightly concerned about the usability aspect in 
that this works
differently from type and search in Dolphin. There's also no visual feedback 
when the search
is reset.

I think it would be a good idea to get some input on this from our usability 
experts.


trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/2659/#comment3205

There's no need to stop the timer here, since calling start() will restart 
it.


- Fredrik


On 2010-01-21 05:09:57, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/2659/
 ---
 
 (Updated 2010-01-21 05:09:57)
 
 
 Review request for Plasma and Fredrik Höglund.
 
 
 Summary
 ---
 
 This patch intends to provide a simple selection method to select icons. Its 
 intended to be able to select a file plasma-desktop.desktop by just typing 
 initial characters, plas for example.
 Comments on the hard-coded 2000ms welcome. If the user doesn't press any key 
 within 2000ms after the last key press, the match string clears.
 
 
 This addresses bug 187241.
 https://bugs.kde.org/show_bug.cgi?id=187241
 
 
 Diffs
 -
 
   trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 1077898 
   trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1077898 
 
 Diff: http://reviewboard.kde.org/r/2659/diff
 
 
 Testing
 ---
 
 Works with the latest trunk.
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Folderview popups should inherit file preview settings

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2286/#review3358
---

Ship it!


Aside from the comment below, it looks good to me.


/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
http://reviewboard.kde.org/r/2286/#comment2696

If you move this call into updateIconViewState(), you only need to do it in 
one place.
That function is called from both configAccepted() and setupIconView().



- Fredrik


On 2009-11-28 09:55:39, Yuen Hoe Lim wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/2286/
 ---
 
 (Updated 2009-11-28 09:55:39)
 
 
 Review request for Plasma and Fredrik Höglund.
 
 
 Summary
 ---
 
 Currently the folderview on-hover popups always previews (only) images no 
 matter what the preview settings of the parent folderview applet is. I saw a 
 'TODO' comment in the code that says popups should inherit file preview 
 settings from the parent, so I assumed this is the right / desired behavior 
 and implemented it :)
 
 I think my approach is sensible, and it would also accommodate allowing the 
 folderview and the popup to have different file preview settings if we ever 
 need that in future. Still, this is my first time ever staring at folderview 
 code, so if I'm doing something unspeakably wrong, or if there's a better way 
 to do this, please let me know :)
 
 (I know it's feature/string freeze now, but this is rectification of faulty 
 behaviour - ie bugfix, so it can be accepted right? This itch happens to 
 bother me somewhat :)
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1055216 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 1055216 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1055216 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/popupview.h 1055216 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/popupview.cpp 1055216 
 
 Diff: http://reviewboard.kde.org/r/2286/diff
 
 
 Testing
 ---
 
 Tested on trunk. Works AFAIK.
 
 
 Thanks,
 
 Yuen Hoe
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Make folderview's hovering popups more usable

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2262/#review3246
---

Ship it!


Part 1 is already implemented in SVN, but part 2 looks good to me.


/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/2262/#comment2571

Stopping the timer once should hopefully be enough (See the line below this 
one).


- Fredrik


On 2009-11-22 19:56:03, Shafqat Bhuiyan wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/2262/
 ---
 
 (Updated 2009-11-22 19:56:03)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This patch adds two things to make the folderview hovering popups more usable:
 1. Don't show popup after user clicks on a folder
 2. Close all popups after user clicks on a file/folder
 
 Part 1 should be good for all users. However part 2 make takes away 
 multi-file opening. I think this is the correct behavior because this is how 
 most people are accustomed to hovering popup views(eg classic application 
 launchers). Also if a user wants to open multiple files he/she can just open 
 the folder with the files inside.
 
 This patch isn't too drastic so it may be backported
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1052895 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/popupview.h 1052895 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/popupview.cpp 1052895 
 
 Diff: http://reviewboard.kde.org/r/2262/diff
 
 
 Testing
 ---
 
 Tested in trunk
 
 
 Thanks,
 
 Shafqat
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Allows selection of icons using the SHIFT key.

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1709/#review2592
---

Ship it!


Looks good to me.

- Fredrik


On 2009-10-08 17:07:37, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/1709/
 ---
 
 (Updated 2009-10-08 17:07:37)
 
 
 Review request for Plasma and Fredrik Höglund.
 
 
 Summary
 ---
 
 If the view is arranged (i.e. m_layoutBroken is false), the icons between the 
 previous and the current click are selected linearly. If the view is broken, 
 the icons which are in the rectangular region of the previous and currently 
 selected icon, are selected. This is because when the view is broken, there 
 is no 'linearity' as such.
 A minor change to key functionality - When a key is pressed, the icons which 
 were previously selected are repainted.
 
 
 This addresses bug 189359.
 https://bugs.kde.org/show_bug.cgi?id=189359
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 1032756 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1032756 
 
 Diff: http://reviewboard.kde.org/r/1709/diff
 
 
 Testing
 ---
 
 Tested on current trunk build. Works fine.
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Make folderview plasmoid to select first item if user press right key. Just like dolphin do.

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1564/#review2281
---

Ship it!


Looks good to me.

- Fredrik


On 2009-09-10 07:44:38, Reza Shah wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/1564/
 ---
 
 (Updated 2009-09-10 07:44:38)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 Make folderview plasmoid to select first item if user press right key. 
 Just like dolphin do.
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1021815 
 
 Diff: http://reviewboard.kde.org/r/1564/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Reza
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Calling KDirLister from plasma widget causes no files to be found.

[Fredrik Höglund]

On Saturday 29 August 2009, David Hubner wrote:
 Hi,
 
 Calling KIO::KDirLister from a plasma widget causes no files to be listed by
 KDirLister. 
 
 An example is the Trash Widget, m_dirlister-openUrl(KUrl(Trash:/)); is
 called but when the signal slotCompleted is thrown,
 m_dirlister-items(KDirLister::AllItems).count() shows 0 files.

KDirLister is able to list files in the folderview applet, although
folderview uses the itemsAdded(), itemsDeleted() etc. signals
indirectly via KDirModel instead of calling KDirLister::items().

There is a bug where KDirLister doesn't emit new items when
a file system is mounted on an empty directory being viewed
by a folderview applet though (#177156). This bug is only
reproducible in Plasma.

Regards,
Fredrik




___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review886
---

Ship it!


Aside from a couple of minor nitpicks below I think the patch looks good now,
so please commit it after taking care of those :)


/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment555

Don't forget to remove this before committing ;)



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment554

Too much indentation here.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment552

Please add a comment here saying that the fall-through is intentional. 



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment556

This closing brace should be at the same level as the else statement on the 
next line.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment553

Too much indentation in this block.


- Fredrik


On 2009-04-08 21:33:17, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/368/
 ---
 
 (Updated 2009-04-08 21:33:17)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This partly addresses the above bug, adding keyboard navigation and launch 
 using Enter key.
 Please report if the code is too complex, I've tried my best to keep it to 
 the point.
 
 
 This addresses bug 187241.
 https://bugs.kde.org/show_bug.cgi?id=187241
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/abstractitemview.h 951365 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/abstractitemview.cpp 
 951365 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 951365 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 951365 
 
 Diff: http://reviewboard.kde.org/r/368/diff
 
 
 Testing
 ---
 
 Tested on latest SVN build. Navigation and launch work fine. The problem is 
 with movement of the scrollbar with the keyboard focus, the scrollbar refuses 
 to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); 
 is used. What am I doing wrong?
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

 On 2009-04-02 13:56:47, Fredrik Höglund wrote:
  I think in general the code looks good, but there are still numerous coding 
  style issues, especially with the way the code is indented.
 
 Shantanu Tushar Jha wrote:
 Oh, I apologise for that, but I'm unable to figure out where I've messed 
 up with the indentation. Please point few examples where there's a problem, 
 it'll be lots of help.

You indent the lines with 2 spaces, while the coding style specifies 4.
The coding style is documented in detail here: 
http://techbase.kde.org/Policies/Kdelibs_Coding_Style


 On 2009-04-02 13:56:47, Fredrik Höglund wrote:
  /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1344
  http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1344
 
  This could result in a division by zero.
 
  wrote:
 The only place where m_columns is set to 0 is the constructor, but after 
 the layout is done, there should be at least one column and one row, then how 
 m_column can be zero? m_columns is used to hold the number of columns 
 visible, isn't it?

The layout isn't done until items are actually inserted into the model, so 
until that happens m_columns will be 0.


 On 2009-04-02 13:56:47, Fredrik Höglund wrote:
  /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1372
  http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1372
 
  This code still doesn't take the flow into account.
  The icons can flow from left to right, right to left, top to bottom and 
  so on, as indicated by m_flow.
 
 
  wrote:
 I can't figure out much what does flow mean here, only that it might mean 
 that the model might be displayed in reverse order for some value of m_flow. 
 What exactly is it doing, is unclear to me as I couldn't find some way to set 
 the flow from the applet config and see the effect. Please hint on this.
 Thanks a lot for the help :)

The flow can only be configured in the config dialog when the applet is used as 
a containment, but basically it controls how the icons are laid out in the 
view, like this: 

LeftToRight:
[1][2][3]
[4][5][6]
[7][8][9]

RightToLeft:
[3][2][1]
[6][5][4]
[9][8][7]

TopToBottom:
[1][4][7]
[2][5][8]
[3][6][9]


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review796
---


On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/368/
 ---
 
 (Updated 2009-04-02 13:21:02)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This partly addresses the above bug, adding keyboard navigation and launch 
 using Enter key.
 Please report if the code is too complex, I've tried my best to keep it to 
 the point.
 
 
 This addresses bug 187241.
 https://bugs.kde.org/show_bug.cgi?id=187241
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 
 
 Diff: http://reviewboard.kde.org/r/368/diff
 
 
 Testing
 ---
 
 Tested on latest SVN build. Navigation and launch work fine. The problem is 
 with movement of the scrollbar with the keyboard focus, the scrollbar refuses 
 to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); 
 is used. What am I doing wrong?
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Tool tips ideas

[Fredrik Höglund]

On Saturday 04 April 2009, Marco Martin wrote:
 On 4/3/09, Aaron J. Seigo ase...@kde.org wrote:
  On Friday 03 April 2009, Emdek wrote:
 
well, the subtitle can actually be html, so there can be a quite good
 level of
 customization (look at the pager tooltip for instance)
   
Yes, I know that, but this helps only in making look them very custom 
  (in
sense of appearance), but doesn't help so much in making them behave
different (for example add actions to them, with icons, but maybe someone
would want to add for example more complex widget in own application)...
 
 
  what are the use cases?
 
   icons are not a problem. actions could be, but then we could probably quite
   easily add a way to get notified of link clicks from tooltips or even allow
   one to send in a list of QActions in ToolTipContent.
 
 and now the question is (and i don't quite have the answer)
 do we really want clickable tooltips or tooltips that just behave as
 well... tooltips?
 (for instance clickable taskbar thumbnails could be handy, but wouuld
 augment the annoyment factor since they won't go away when the mouse
 leaves the task area)

Being interactive doesn't mean that the tooltips have to stick
around until the user explicitly closes them.

The tooltips the folderview applet pops up when you hover a folder
icon contain icon views that you can interact with, and tooltips
will even open recursively for files inside that tooltip.

But they still go away when the cursor stops hovering the folder
icon or when the cursor leaves the tooltip. The delay before they're
hidden means that the user has time to move the cursor into the
tooltip if they want to interact with it.

Those tooltips are not implemented using Plasma::ToolTipManager,
and I'm not sure if they should be. The recursive nature of the
tooltips makes the mechanism that determines when a tip should
be closed quite complicated.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

 On 2009-03-20 14:07:32, Fredrik Höglund wrote:
  /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
  http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
 
  A problem with the way this function is implemented is that it assumes 
  that the view is always sorted and that the icons always flow from left to 
  right.
  
  When the user has rearranged the icons (m_layoutBroken is true), you 
  have to assume that the icons are no longer arranged in a grid and that the 
  visual order no longer matches the order in the model.
  
  When this is the case, and the user has pressed the up key for example, 
  you have to iterate over all the icons and find the one that is closest to 
  the current icon while still being above it.
 
 
  wrote:
 you have to iterate over all the icons. I'm working on this by 
 iterating all icons and finding the nearest one to the current selection 
 according to the key pressed, but the code is getting really complex in terms 
 of calculations. I was wondering if there is any other way of doing this? If 
 anyone has an idea, please let me know. Till then I'm working on it.

 
  wrote:
 I would do something like this (the example is for the up key only):
 
 QModelIndex nextIndex = QModelIndex();
 QPoint currentPos = visualRect(currentIndex).center();
 int lastDistance = 0;
 
 for (int i = 0; i  m_validRows; i++) {
  const QModelIndex index = m_model-index(i, 0);
  const QPoint pos = visualRect(index).center();
  if (pos.y()  currentPos.y()) {
  int distance = (pos - currentPos).manhattanLength();
  if (distance  lastDistance || !currentIndex.isValid()) {
  nextIndex = index;
  lastDistance = distance;
  }
  }
 }
 
 If nextIndex is valid when you get here, it's the index you should move 
 to.
 If it isn't valid there are no icons above the current icon.
 
 Thanks for working on this feature :)

 
  wrote:
 Ok, thanks for the help, that was less complex then mine ;) Its almost 
 done, just a few minor issues remaining.
 But, I'm unable to understand why you're using `!currentIndex.isValid()` 
 in if (distance  lastDistance || !currentIndex.isValid()) ?
 Why do we need to validate the currentIndex ?

Oh right, that should actually be nextIndex, not currentIndex.
I guess should read what I wrote more carefully before pressing publish :)


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---


On 2009-04-02 08:55:49, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/368/
 ---
 
 (Updated 2009-04-02 08:55:49)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This partly addresses the above bug, adding keyboard navigation and launch 
 using Enter key.
 Please report if the code is too complex, I've tried my best to keep it to 
 the point.
 
 
 This addresses bug 187241.
 https://bugs.kde.org/show_bug.cgi?id=187241
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 947761 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 947761 
 
 Diff: http://reviewboard.kde.org/r/368/diff
 
 
 Testing
 ---
 
 Tested on latest SVN build. Navigation and launch work fine. The problem is 
 with movement of the scrollbar with the keyboard focus, the scrollbar refuses 
 to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); 
 is used. What am I doing wrong?
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review796
---


I think in general the code looks good, but there are still numerous coding 
style issues, especially with the way the code is indented.


/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h
http://reviewboard.kde.org/r/368/#comment486

I'd prefer it if this function was in AbstractItemView instead, since the 
code will work for the ListView class as well.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment489

IconView already sets the focus policy to StrongFocus at the top of the 
constructor (as of today).




/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment487

This could result in a division by zero.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment488

This code still doesn't take the flow into account.
The icons can flow from left to right, right to left, top to bottom and so 
on, as indicated by m_flow.



- Fredrik


On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/368/
 ---
 
 (Updated 2009-04-02 13:21:02)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This partly addresses the above bug, adding keyboard navigation and launch 
 using Enter key.
 Please report if the code is too complex, I've tried my best to keep it to 
 the point.
 
 
 This addresses bug 187241.
 https://bugs.kde.org/show_bug.cgi?id=187241
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 
 
 Diff: http://reviewboard.kde.org/r/368/diff
 
 
 Testing
 ---
 
 Tested on latest SVN build. Navigation and launch work fine. The problem is 
 with movement of the scrollbar with the keyboard focus, the scrollbar refuses 
 to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); 
 is used. What am I doing wrong?
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

 On 2009-03-20 14:07:32, Fredrik Höglund wrote:
  /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
  http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
 
  A problem with the way this function is implemented is that it assumes 
  that the view is always sorted and that the icons always flow from left to 
  right.
  
  When the user has rearranged the icons (m_layoutBroken is true), you 
  have to assume that the icons are no longer arranged in a grid and that the 
  visual order no longer matches the order in the model.
  
  When this is the case, and the user has pressed the up key for example, 
  you have to iterate over all the icons and find the one that is closest to 
  the current icon while still being above it.
 
 
  wrote:
 you have to iterate over all the icons. I'm working on this by 
 iterating all icons and finding the nearest one to the current selection 
 according to the key pressed, but the code is getting really complex in terms 
 of calculations. I was wondering if there is any other way of doing this? If 
 anyone has an idea, please let me know. Till then I'm working on it.

 
  wrote:
 I would do something like this (the example is for the up key only):
 
 QModelIndex nextIndex = QModelIndex();
 QPoint currentPos = visualRect(currentIndex).center();
 int lastDistance = 0;
 
 for (int i = 0; i  m_validRows; i++) {
  const QModelIndex index = m_model-index(i, 0);
  const QPoint pos = visualRect(index).center();
  if (pos.y()  currentPos.y()) {
  int distance = (pos - currentPos).manhattanLength();
  if (distance  lastDistance || !currentIndex.isValid()) {
  nextIndex = index;
  lastDistance = distance;
  }
  }
 }
 
 If nextIndex is valid when you get here, it's the index you should move 
 to.
 If it isn't valid there are no icons above the current icon.
 
 Thanks for working on this feature :)

 
  wrote:
 Ok, thanks for the help, that was less complex then mine ;) Its almost 
 done, just a few minor issues remaining.
 But, I'm unable to understand why you're using `!currentIndex.isValid()` 
 in if (distance  lastDistance || !currentIndex.isValid()) ?
 Why do we need to validate the currentIndex ?
 
  wrote:
 Oh right, that should actually be nextIndex, not currentIndex.
 I guess should read what I wrote more carefully before pressing publish 
 :)
 
  wrote:
 But still, why we need to validate (or, invalidate) nextIndex 
 `!currentIndex.isValid()` ? I tried to figure out, but failed. A little help 
 here :)

Because in the first iteration of the loop, lastDistance is 0, so 'distance  
lastDistance' will be false.
Adding '|| nextIndex.isValid()' causes nextIndex and lastDistance to be 
initialized to the values for the first item in the view.

Assigning QModelIndex() to an index makes the index invalid.


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---


On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/368/
 ---
 
 (Updated 2009-04-02 13:21:02)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This partly addresses the above bug, adding keyboard navigation and launch 
 using Enter key.
 Please report if the code is too complex, I've tried my best to keep it to 
 the point.
 
 
 This addresses bug 187241.
 https://bugs.kde.org/show_bug.cgi?id=187241
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 
 
 Diff: http://reviewboard.kde.org/r/368/diff
 
 
 Testing
 ---
 
 Tested on latest SVN build. Navigation and launch work fine. The problem is 
 with movement of the scrollbar with the keyboard focus, the scrollbar refuses 
 to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); 
 is used. What am I doing wrong?
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

 On 2009-03-20 14:07:32, Fredrik Höglund wrote:
  /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
  http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
 
  A problem with the way this function is implemented is that it assumes 
  that the view is always sorted and that the icons always flow from left to 
  right.
  
  When the user has rearranged the icons (m_layoutBroken is true), you 
  have to assume that the icons are no longer arranged in a grid and that the 
  visual order no longer matches the order in the model.
  
  When this is the case, and the user has pressed the up key for example, 
  you have to iterate over all the icons and find the one that is closest to 
  the current icon while still being above it.
 
 
  wrote:
 you have to iterate over all the icons. I'm working on this by 
 iterating all icons and finding the nearest one to the current selection 
 according to the key pressed, but the code is getting really complex in terms 
 of calculations. I was wondering if there is any other way of doing this? If 
 anyone has an idea, please let me know. Till then I'm working on it.


I would do something like this (the example is for the up key only):

QModelIndex nextIndex = QModelIndex();
QPoint currentPos = visualRect(currentIndex).center();
int lastDistance = 0;

for (int i = 0; i  m_validRows; i++) {
 const QModelIndex index = m_model-index(i, 0);
 const QPoint pos = visualRect(index).center();
 if (pos.y()  currentPos.y()) {
 int distance = (pos - currentPos).manhattanLength();
 if (distance  lastDistance || !currentIndex.isValid()) {
 nextIndex = index;
 lastDistance = distance;
 }
 }
}

If nextIndex is valid when you get here, it's the index you should move to.
If it isn't valid there are no icons above the current icon.

Thanks for working on this feature :)


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---


On 2009-03-20 22:14:51, Shantanu Tushar Jha wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.kde.org/r/368/
 ---
 
 (Updated 2009-03-20 22:14:51)
 
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This partly addresses the above bug, adding keyboard navigation and launch 
 using Enter key.
 Please report if the code is too complex, I've tried my best to keep it to 
 the point.
 
 
 This addresses bug 187241.
 https://bugs.kde.org/show_bug.cgi?id=187241
 
 
 Diffs
 -
 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 942106 
   /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 942106 
 
 Diff: http://reviewboard.kde.org/r/368/diff
 
 
 Testing
 ---
 
 Tested on latest SVN build. Navigation and launch work fine. The problem is 
 with movement of the scrollbar with the keyboard focus, the scrollbar refuses 
 to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); 
 is used. What am I doing wrong?
 
 
 Thanks,
 
 Shantanu
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Plasma, ARGB and 4.5

[Fredrik Höglund]

On Wednesday 18 February 2009, Marco Martin wrote:
 On Tuesday 17 February 2009, Fredrik Höglund wrote:
 
   seems to work quite good, except for two things:
   the tooltips seems to insist that they don't want to be transparent
   the systemtray has garbage again with the raster graphicssystem (so i
   didn't dream it up) i'm not sure if that's the case also with the old
   method btw
 
  Reverting r907753 should fix the systray icons.
 
  You'll have to read my reply on kde-commits for the details, but the short
  version is that the move to using Qt::WA_TranslucentBackground combined
  with that patch means that Plasma is now explicitly telling Qt (and Gtk) to
  not use real transparency for the systray icons.
 
 the strange thing is that i don't get junk with x11 graphics system,
 while i get it for all icons with raster graphicssystem.
 reverting that patch it fixes only qt4 icons but not qt3 and gtk ones

It looks like there's still an assumption in the systray in the non-
composited case that a QPixmap is an X pixmap.

The bad news is that it doesn't look trivial to fix.

 by the way, could make sense something like this?
 forces 32 bits only when there are more than 16

The patch ddenis submitted assumes that the X server creates an ARGB
visual for each depth and that Plasma should choose the one that
corresponds to the display depth.

That assumption is wrong; the X server only creates one ARGB visual, it's
always 32 bits and it works regardless of the display depth.

So the code that was there before the patch was committed was correct.
The patch didn't fix anything because there's nothing to fix, but it broke
things that weren't broken before.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Plasma, ARGB and 4.5

[Fredrik Höglund]

On Monday 16 February 2009, Marco Martin wrote:
 On Monday 16 February 2009, Aaron J. Seigo wrote:
  On Friday 13 February 2009, Marco Martin wrote:
   Hi all,
   right now plasma enables argb visuals with that magin X11 code pasted in
   many places...
   since 4.5 now can handle it
   by setting Qt::WA_TranslucentBackground on the top level window we want
   transparent we can make it in a prettier way
 
  that would be nice.
 done,
 seems to work quite good, except for two things:
 the tooltips seems to insist that they don't want to be transparent
 the systemtray has garbage again with the raster graphicssystem (so i didn't  
 dream it up) i'm not sure if that's the case also with the old method btw

Reverting r907753 should fix the systray icons.

You'll have to read my reply on kde-commits for the details, but the short
version is that the move to using Qt::WA_TranslucentBackground combined
with that patch means that Plasma is now explicitly telling Qt (and Gtk) to
not use real transparency for the systray icons.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Make folderview follow some guidelines

[Fredrik Höglund]

On Sunday 14 December 2008 16:34, Artur Souza (MoRpHeUz) wrote:
 Hey!
 
 On Sun, Dec 14, 2008 at 12:00 PM, Fredrik Höglund fred...@kde.org wrote:
  The warning that the desktop folder will be created when the user selects
  Show Desktop folder in the config dialog was carefully worked out in
  coordination with our usability experts, so I thank you for not removing it.
 
 That's why we are discussing this here and didnt commit
 anything...from aseigo's point of view it shouldnt never create
 automatically the folder thus making the warning useless.

It doesn't create the folder unless the user explicitly asks it do so, hence
the warning informing the user about what will happen when they select
that option.

The alternative is that we gray out the option, and if the user wants
to enable it they will have to start systemsettings, click About Me
(the most logical place where one would look for the desktop path), figure
out what the folder should be named, create it, then copy all the files in
${PREFIX}/share/apps/kio_desktop/DesktopLinks into the folder, copy
directory.desktop and directory.trash from kio_desktop into it and
rename them to .directory and trash.desktop respectively.

We could add those instructions to the What's This help in KDE 4.3, but
I'm against that idea because I think the competition would be laughing
their asses off.

Or we could do all this for the user when they've selected that option,
and made it clear that it is what they want.

 And about the patch itself, a lot of discussion happened in #plasma
 with two different groups and too diferent ideas in two different
 days. The bug reports and these discussions shows that we need to do
 polish some stuff here.

Agreed.

  Your patch doesn't change the fact that the folder will be created when
  the user selects that option, you've merely changed the default preference
  when the user hasn't configured anything.
 
 The folder isnt created when it doesnt exist, just tested that. It
 shows a big red X with the label: The file or folder XXX does not
 exist. Can you tell the steps to reproduce it here ?

If you look at the top of configAccepted() you'll see that it sets the URL
to desktop:/ when the Show Desktop folder option is checked.

The desktop ioslave should do a check and create the folder if it doesn't
exist, but it's possible something has regressed or that you've found a bug.

I can't reproduce it here though.

 Ah, btw I couldnt find your username at reviewboard and thus couldnt
 cc you the patch.

I don't have an account on the reviewboard, but I'll save that for another
discussion :)

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Make folderview follow some guidelines

[Fredrik Höglund]

On Sunday 14 December 2008 17:09, Celeste Lyn Paul wrote:
 On Sunday 14 December 2008 10:34:22 Artur Souza (MoRpHeUz) wrote:
  Hey!
 
  On Sun, Dec 14, 2008 at 12:00 PM, Fredrik Höglund fred...@kde.org wrote:
   The warning that the desktop folder will be created when the user selects
   Show Desktop folder in the config dialog was carefully worked out in
   coordination with our usability experts, so I thank you for not removing
   it.
 
  That's why we are discussing this here and didnt commit
  anything...from aseigo's point of view it shouldnt never create
  automatically the folder thus making the warning useless.
 
 Because he doesn't want to promote the use of the Desktop folder? Personally, 
 I agree and strongly dislike the use of the Desktop folder, but it is still 
 part of the xdg folder spec isnt' it?  I think it is a better idea to 
 actively 
 support it than allow users to configure it passively as another folder. But 
 whatever the boss says I guess.

While slightly off-topic in the discussion, I think it's important that when
we want to change cross desktop standards that we do this in collaboration
with the other desktops, and don't unilaterally start ignoring standards we've
helped create, but now decided that we don't like anymore.

If we don't do that then we end up creating problems for users and ISV's alike.

An example of such a problem in this case is that Firefox saves all
downloaded files in the desktop folder by default. If we deny users access
to it, then they won't be able to find those files.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Bugfix for Folderview bug #175191

[Fredrik Höglund]

On Sunday 30 November 2008 18:20, Aaron Seigo wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.vidsolbach.de/r/288/#review276
 ---
 
 Ship it!
 
 
 yes, that's probably sensible.
 
 
 /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
 http://reviewboard.vidsolbach.de/r/288/#comment228
 
 i'm a little surprised that setUrl doesn't call dirListed()-openUrl or 
 updateIconWidget, but Frederik could probably explain why =)

I actually can't explain that because I didn't write that particular method ;)

But I think it's a bit of a misnomer because what it really does is update
the icon view label.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: style on the taskbar group menu

[Fredrik Höglund]

On Monday 03 November 2008 17:58, Marco Martin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.vidsolbach.de/r/254/
 ---
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 this makes the taskbar group menu to draw mostly like an extender, to 
 implement a mockup of the new theme
 it's just a subclass of the menu with custom painting, still think in the 
 future should be  the real tasks widgets within a view, but i kinda feel it's 
 a 4.3 thing :)

With the text color hardcoded to white, this code will only work with dark 
themes.

I also don't think it's a good idea to abuse the tasks-hover background for the
highlighted item, since this imposes a significant design limitation on the 
theme
in that the taskbar button background has to be drawn in such a way that it also
works as the highlighted menu background. I think the theme should provide a
separate element for highlighted menu items.

Looking at other systems it seems that Vista uses the same theme element for
selected icons, items in treeviews, and popup menus.

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: style on the taskbar group menu

[Fredrik Höglund]

On Monday 03 November 2008 23:33, Marco Martin wrote:
 On Monday 03 November 2008, Fredrik Höglund wrote:
  I also don't think it's a good idea to abuse the tasks-hover background for
  the highlighted item, since this imposes a significant design limitation on
  the theme in that the taskbar button background has to be drawn in such a
  way that it also works as the highlighted menu background. I think the
  theme should provide a separate element for highlighted menu items.
 the idea was to resemble the task entries because in the future that would 
 probably become a graphicsview with the actual tasks inside it
 just an idea maybe won't be done in that way, but the idea was to make it 
 more 
 look like the taskbar than a menu...
 if in the end will be decided to leave it as a menu like that yeah, an 
 element 
 just for menus could make sense yeah.

I was thinking of consistency with other applets that open up similar menus.
The folderview applet will do this when it's on the panel for instance.

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Spinner widget

[Fredrik Höglund]

On Monday 03 November 2008 15:25, Marco Martin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviewboard.vidsolbach.de/r/253/
 ---
 
 Review request for Plasma.
 
 
 Summary
 ---
 
 This is the spinner widget we talked about in the last irc meeting, would 
 like to make in 4.2..
 it's api is at the bare minimum (no api at all basically :p) could have 
 things like strt/stop or speed settings, but not sure it's necessary...
 i'm not really sure about the folderview part, it's done rather quickly as a 
 test :)

One thing I don't like about this code is that it rotates the painter before it
draws the FrameSvg. QPainter will apply the transformation to the pixmap
in software, meaning it will convert it to an image, rotate it, convert it back
to a pixmap, and then drawn it. This makes showing the busy animation
expensive in itself. In my opinion the Spinner widget needs to keep a cached
copy of each frame in the animation.

It also looks to me like the code assumes that there is a shadow element,
but I'm not sure it should be a requirement for the theme to provide one.
The offset for the shadow element is also hardcoded.

The nativeWidget() function doesn't make much sense to me, since the spinner
is a native QGraphicsWidget, and will never be based on a QWidget.

I also don't think it makes much sense to have an empty setStyleSheet()
function, since it can be added at any time in the future without breaking
binary compatibility.

I also think Applet should have a setShowBusyIndicator(bool on) that takes
care of showing and hiding the spinner, so the code for it won't have to be
duplicated in each applet that needs to use it.

Other than that the code looks good :)

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: QuickAccess Plasmoid

[Fredrik Höglund]

On Wednesday 29 October 2008 03:50, Aaron J. Seigo wrote:
 On Tuesday 28 October 2008, Loïc Marteau wrote:
  http://www.kde-look.org/content/show.php?content=84128
 
  I think than this one is really a killer one and should me moved
 
 ignoring the various ways it ignores plasma conventions in the code, isn't 
 this simply what folderview should be in a horiz/vert constrained view?
 
 in that sense it suffers from the opposite problem folderview does: it's only 
 for horiz/vert form factors ;)

I have code for that, but it's not in a state where it's ready to
be committed yet.

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: QuickAccess Plasmoid

[Fredrik Höglund]

On Wednesday 29 October 2008 15:07, Sebastian Kügler wrote:
 On Wednesday 29 October 2008 08:11:00 Loïc Marteau wrote:
  What do you suggest ?
 
 Making folderview a popupapplet as first step and solving interaction 
 problems 
 from there.

You can't turn a Containment into a PopupApplet, because PopupApplet
doesn't inherit Containment.

Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: playground applets

[Fredrik Höglund]

On Tuesday 30 September 2008 06:33, Aaron J. Seigo wrote:
 needs more work
 ===
 train-clock -  sizing oddnesses (see difference between clock size and the 
 standard background)
 commandwatch - should use exec dataengine; should use a scrolling text area; 
 wrong PluginInfo-Name style
 contacts  - should require config on 0 contacts; contacts sould be clickable; 
 scrollable area for items; in panel, a paged mode?
 eyes - what would life be without eyes? ;) needs oxygenation though; too old 
 school as it is

I think this applet should also use the mouse dataengine, but I may be
slightly biased here since I wrote it ;)

It should keep the number of synchronous requests down when you have
several applets that need to track the global mouse position though.

Regards,
Fredrik

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel