D15357: [Bookmarks Runner] Remove duplicate results for bookmarks

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:e02f3fcb1347: [Bookmarks Runner] Remove duplicate results 
for bookmarks (authored by bruns).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15357?vs=42615=42768

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

AFFECTED FILES
  runners/bookmarks/browsers/firefox.cpp

To: bruns, #plasma, broulik
Cc: broulik, davidedmundson, zzag, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15892: [Devicenotifications Engine] Keep at most one notification per UDI

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
bruns marked an inline comment as done.
Closed by commit R120:17380886d9b8: [Devicenotifications Engine] Keep at most 
one notification per UDI (authored by bruns).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D15892?vs=42702=42767#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15892?vs=42702=42767

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

AFFECTED FILES
  dataengines/devicenotifications/devicenotificationsengine.cpp
  dataengines/devicenotifications/devicenotificationsengine.h
  dataengines/devicenotifications/ksolidnotify.cpp
  dataengines/devicenotifications/ksolidnotify.h

To: bruns, #frameworks, broulik
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15907: Compare float values in DecorationButton contains check

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> zzag wrote in decorationbutton.cpp:455
> Can you please explain this part? Shouldn't it be `d->geometry.right() < 
> pos.x()`?

If `rect.width()` is smaller 0, then arbitrary QRect `rect` is to the left of 
its "anchor point" `rect.x()`. The right border is therefore defined by value 
`rect.x()`.

Looking at the Qt source in this case `rect.right()` defines the left border of 
the rectangle. I assume we always want to exclude the border to the right. I 
also hope that no on in practice defines a decoration button with negative 
width. ;)

REPOSITORY
  R129 Window Decoration Library

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

To: romangg, #kwin, zzag, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15907: Compare float values in DecorationButton contains check

[Vlad Zagorodniy]

zzag added a comment.


  Also, fwiw, comparing floating point numbers with the plain `<` is not quite 
correct(e.g. 0.3 < 0.1 + 0.2 vs 0.1 + 0.2 < 0.3). Qt does the same so I guess 
that's fine.

INLINE COMMENTS

> decorationbutton.cpp:455
> +// additional make sure pos is not on the right or bottom edge
> +const bool verInside = d->geometry.width() < 0 ? pos.x() < 
> d->geometry.x() :
> + pos.x() < 
> d->geometry.right();

Can you please explain this part? Shouldn't it be `d->geometry.right() < 
pos.x()`?

REPOSITORY
  R129 Window Decoration Library

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

To: romangg, #kwin, zzag, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15907: Compare float values in DecorationButton contains check

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  This restores the bug the last patch fixed.

REPOSITORY
  R129 Window Decoration Library

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

To: romangg, #kwin, zzag, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15907: Compare float values in DecorationButton contains check

[Roman Gilg]

romangg created this revision.
romangg added reviewers: KWin, zzag, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
romangg requested review of this revision.

REVISION SUMMARY
  In c9cfd840137b 
 a 
position contains check was introduced in order to not count
  a bottom or right edge position as being part of the DecorationButton like
  QRectF::contains does.
  
  This was done by converting the geometry to a QRect and using its contains
  function, which does the comparision the desirable way for legacy reasons.
  
  But this means we lose precision, since the decoration library in theory
  supports floating values for positioning.
  
  To not do this instead of using QRect's functionality and implicitly dropping
  float precision therefore do the check for bottom and right edge explicitly.

TEST PLAN
  Adapted autotest.

REPOSITORY
  R129 Window Decoration Library

BRANCH
  decorationContainsFix

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

AFFECTED FILES
  autotests/decorationbuttontest.cpp
  src/decorationbutton.cpp

To: romangg, #kwin, zzag, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14949: Add option for whether to show the volume change OSD

[Nathaniel Graham]

ngraham added a comment.


  Listening to user feedback is important. We not only have complaints on 
Reddit, but also someone was motivated enough to submit a patch here. I've also 
seen it brought up in Bugzilla tickets.

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

To: anonym, #vdg
Cc: graesslin, svenmauch, ngraham, romangg, mart, broulik, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D7038: [server] Respect input region of sub-surfaces on pointer surface focus

[David Edmundson]

davidedmundson added a comment.


  I think maybe it should, but it should be changed with surfaceAt

REPOSITORY
  R127 KWayland

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

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

D7038: [server] Respect input region of sub-surfaces on pointer surface focus

[Vlad Zagorodniy]

zzag added a comment.


  > QRectF(QPoint(0, 0), size()).contains(position)
  
  Shouldn't it be QRect instead? (again)
  If it should be QRectF indeed, could someone please explain why?

REPOSITORY
  R127 KWayland

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

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

D7038: [server] Respect input region of sub-surfaces on pointer surface focus

[Roman Gilg]

romangg updated this revision to Diff 42733.
romangg added a comment.


  - Update version number
  - Do code duplication instead of pointers
  - Add comment about code duplication

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7038?vs=42725=42733

BRANCH
  inputRegionSubSurfaces

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

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

D15637: Make DBusMenu work correctly with dynamically generated menus

[Igor Poboiko]

poboiko added a comment.


  In D15637#335295 , @broulik wrote:
  
  > From what I can tell this patch trades one bug (menu closing unexpectedly) 
for another one (menus erratically resizing/repositioning themselves).
  
  
  As for LyX it's not just menu closing unexpectedly, but it's just totally 
unusable: I cannot access menu using the applet at all. That was the main 
reason I started looking into it.
  
  But as for glitches: what about proposed `QWidget::setUpdatesEnabled` calls? 
(funny, actually it doesn't work for LyX, because, apparently, when I open a 
menu, it decides to run GetLayout 5-10 times in a row, updating the menu each 
time and causing glitches. Weird, and I don't even know who is responsible for 
that)
  
  > I think we should be updating the actions in lockstep rather than adding 
all the things and then removing the old ones or vice-versa.
  
  That does sound like a better way. But I don't really know how do updated 
items correspond with old ones, since they have different IDs.
  In principle those can represent absolutely different actions - if 
application really decided to totally mess with the menu during runtime.

REPOSITORY
  R120 Plasma Workspace

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

To: poboiko, #plasma, broulik, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D7038: [server] Respect input region of sub-surfaces on pointer surface focus

[Roman Gilg]

romangg updated this revision to Diff 42725.
romangg added a comment.


  Rebase on master.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7038?vs=18059=42725

BRANCH
  inputRegionSubSurfaces

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h
  src/server/surface_interface_p.h

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

D15877: [KonsoleProfiles applet] Fix navigating with the keyboard

[Thomas Surrel]

thsurrel added a comment.


  I don't have a developer account, could you land this ? Thanks

REPOSITORY
  R114 Plasma Addons

BRANCH
  arc_konsoleprofiles (branched from master)

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

To: thsurrel, #plasma, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15512: [startplasmacompositor] Add Wayland socket argument

[Roman Gilg]

romangg added a comment.


  On the other side does it sometimes make sense to specify a port manually 
even if KWin could take the next free one? I would say yes if I have a client 
to test against, which I can provide with a socket argument (I don't need to 
query which free socket actually was taken by KWin). So in the end we want to 
have both methods anyway.

REPOSITORY
  R120 Plasma Workspace

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

To: romangg, #kwin, #plasma
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15823: [Folder View] In list view mode, fix home button disappearing every other subfolder entered

[Andres Betts]

abetts added a comment.


  In D15823#335296 , @thsurrel wrote:
  
  > F6299154: screenshot.png 
  >  Showing the missing home button in the top left corner. The button would 
appear again if I would navigate one folder up or down.
  
  
  Thank you

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arc_folderview (branched from master)

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

To: thsurrel, #plasma, #vdg, hein
Cc: abetts, hein, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D15823: [Folder View] In list view mode, fix home button disappearing every other subfolder entered

[Thomas Surrel]

thsurrel added a comment.


  F6299154: screenshot.png 
  Showing the missing home button in the top left corner. The button would 
appear again if I would navigate one folder up or down.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arc_folderview (branched from master)

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

To: thsurrel, #plasma, #vdg, hein
Cc: abetts, hein, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D15637: Make DBusMenu work correctly with dynamically generated menus

[Kai Uwe Broulik]

broulik added a comment.


  From what I can tell this patch trades one bug (menu closing unexpectedly) 
for another one (menus erratically resizing/repositioning themselves).
  I think we should be updating the actions in lockstep rather than adding all 
the things and then removing the old ones or vice-versa.

REPOSITORY
  R120 Plasma Workspace

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

To: poboiko, #plasma, broulik, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15823: [Folder View] In list view mode, fix home button disappearing every other subfolder entered

[Andres Betts]

abetts added a comment.


  Screenshot?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arc_folderview (branched from master)

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

To: thsurrel, #plasma, #vdg, hein
Cc: abetts, hein, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D15892: [Devicenotifications Engine] Keep at most one notification per UDI

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> broulik wrote in devicenotificationsengine.cpp:39
> This potentially breaks applets relying on the structure of that name? Not 
> sure how big of an issue this is as far as "API promise" we give for 
> dataengines but perhaps at least keep the `notification` part first

The only user I could find is the device notifier, which relies on the contents 
of the container only for matching, and does not require any specific format 
for the source name.
Preferably we had some API documentation for the dataengines ...

> broulik wrote in devicenotificationsengine.h:26
> Unused?

yes, leftover from a different approach.

> devicenotificationsengine.h:43
>   void notify(Solid::ErrorType solidError, const QString& error, const 
> QString& errorDetails, const QString );
> +void clearNotification(const QString );
>  

whitespace.

> broulik wrote in ksolidnotify.cpp:152
> Move this into the relevant `case` below, please

I considered this and did not because of the early return.
All the other `case`s just do a break and have a common return statement (more 
correctly, an `emit notify(...)`), this one differs.

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #frameworks
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15512: [startplasmacompositor] Add Wayland socket argument

[Fabian Vogt]

fvogt added a comment.


  In D15512#335264 , @romangg wrote:
  
  > Is there a real reason against it besides "not necessary"? Maybe that we 
can't remove the manual override again without breaking some API promise once 
we have the grand solution of automatically taking the next free Wayland socket?
  
  
  IMO "not necessary" is a real reason.
  That this adds public API is also a reason not to add this, especially 
because it will be fully superseded by the next free socket search.
  
  > Otherwise there is no damage in adding this parameter and I can avoid 
having my local git checkout diverge from master.
  
  The use case for this is to not conflict with an already existing wayland 
display. As this only works if launched manually with a special parameter, it 
only works for the minority of users, which makes this a workaround or even 
hack.
  AFAICT (but I might be wrong) implementing the next free socket function 
shouldn't be more lines than this diff is and work in all cases and without 
user interaction.

REPOSITORY
  R120 Plasma Workspace

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

To: romangg, #kwin, #plasma
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14949: Add option for whether to show the volume change OSD

[Sven Mauch]

svenmauch added a comment.


  In D14949#334937 , @graesslin 
wrote:
  
  > Please don't introduce options based on what's unpopular on Reddit.
  
  
  I'm not suggesting we should base new options entirely off reddit opinions. 
That would be short-sighted. :)
  
  However I see no harm in adding to this discussion that changes or new 
options to OSD would be welcomed by a number people. Certainly falls under the 
category "quick and dirty user research" but then again, why disregard it 
completely?
  
  From what I gathered so far there is a need (more or less) for a less 
obstrusive and a completely hidden OSD. I'd get behind three options so we 
don't overcomplicate things: classic, compact, none.
  
  Of course I'm open to arguments why we shouldn't do this. :)

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

To: anonym, #vdg
Cc: graesslin, svenmauch, ngraham, romangg, mart, broulik, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D15512: [startplasmacompositor] Add Wayland socket argument

[Roman Gilg]

romangg added a comment.


  Is there a real reason against it besides "not necessary"? Maybe that we 
can't remove the manual override again without breaking some API promise once 
we have the grand solution of automatically taking the next free Wayland socket?
  
  Otherwise there is no damage in adding this parameter and I can avoid having 
my local git checkout diverge from master.

REPOSITORY
  R120 Plasma Workspace

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

To: romangg, #kwin, #plasma
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15823: [Folder View] In list view mode, fix home button disappearing every other subfolder entered

[Thomas Surrel]

thsurrel added a comment.


  I don't have a developer account, can you be the pilot for landing this ? 
Thanks in advance.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arc_folderview (branched from master)

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

To: thsurrel, #plasma, #vdg, hein
Cc: hein, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15637: Make DBusMenu work correctly with dynamically generated menus

[Igor Poboiko]

poboiko added a comment.


  Ping?

REPOSITORY
  R120 Plasma Workspace

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

To: poboiko, #plasma, broulik, davidedmundson
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15887: Fix Klipper popup opening on default screen instead of at cursor position

[Roman Geints]

romangeints updated this revision to Diff 42713.
romangeints added a comment.


  Renamed a variable, removed extra parentheses

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15887?vs=42689=42713

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

AFFECTED FILES
  klipper/klipper.cpp

To: romangeints, #plasma, davidedmundson
Cc: romangeints, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15823: [Folder View] In list view mode, fix home button disappearing every other subfolder entered

[Eike Hein]

hein accepted this revision.
hein added a comment.
This revision is now accepted and ready to land.


  Nice catch!

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arc_folderview (branched from master)

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

To: thsurrel, #plasma, #vdg, hein
Cc: hein, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15616: [Comic] Handle error state correctly

[Anthony Fieroni]

anthonyfieroni added a comment.


  Ping

REPOSITORY
  R114 Plasma Addons

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

To: anthonyfieroni, davidedmundson, #plasma, broulik
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15357: [Bookmarks Runner] Remove duplicate results for bookmarks

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.
This revision is now accepted and ready to land.


  Somehow the usage of `equal_range` and `keyValueBegin` looks quite convoluted 
but if it works... ship it

REPOSITORY
  R120 Plasma Workspace

BRANCH
  T9626

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

To: bruns, #plasma, broulik
Cc: broulik, davidedmundson, zzag, ngraham, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15892: [Devicenotifications Engine] Keep at most one notification per UDI

[Kai Uwe Broulik]

broulik added a comment.


  I think this makes sense

INLINE COMMENTS

> devicenotificationsengine.cpp:39
>  {
> -const QString source = QStringLiteral("notification %1").arg(m_id++);
> +const QString source = QStringLiteral("%1 notification").arg(udi);
>  

This potentially breaks applets relying on the structure of that name? Not sure 
how big of an issue this is as far as "API promise" we give for dataengines but 
perhaps at least keep the `notification` part first

> devicenotificationsengine.h:26
>  #include 
> +#include 
>  

Unused?

> ksolidnotify.cpp:152
>  {
> +if ((error == Solid::ErrorType::NoError) && (type == 
> SolidReplyType::Setup)) {
> +emit clearNotification(udi);

Move this into the relevant `case` below, please

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #frameworks
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart