D16852: Add Documents to the default list of Places

[Andrew Crouthamel]

acrouthamel added a comment.


  I checked out the test log and see the following, yet I honestly can't figure 
out why it is off by one, since any line referencing hasDocumentsFolder is a ++ 
line for the count. Do I need to run this test on a new user account or 
something?
  
FAIL!  : PlacesItemModelTest::testDeletePlace() Compared lists differ at 
index 2.
   Actual   (places): "/home/andrew/Desktop"
   Expected (urls): "/home/andrew/Documents"
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(411)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Places - Home) Compared values 
are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Baloo - Documents) Compared 
values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Baloo - Today) Compared values 
are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testPlaceItem(Devices - Floppy) Compared 
values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testTearDownDevice() Compared values are not 
the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Places - Home) 
Compared values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Baloo - Documents) 
Compared values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Places - Audio) 
Compared values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Baloo - Today) 
Compared values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testDefaultViewProperties(Devices - Floppy) 
Compared values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testClear() Compared values are not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testHideItem() Compared values are not the 
same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testSystemItems() Compared values are not the 
same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testEditBookmark() Compared values are not 
the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testEditAfterCreation() Compared values are 
not the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   Loc: 
[/home/andrew/kde/src/kde/applications/dolphin/src/tests/placesitemmodeltest.cpp(245)]
FAIL!  : PlacesItemModelTest::testEditMetadata() Compared values are not 
the same
   Actual   (m_model->count()): 19
   Expected (m_expectedModelCount): 18
   

D16905: Add a generic sorting options icon

[Nathaniel Graham]

ngraham added a comment.


  +1 to everything @ndavis is saying. I love the new double-arrow versions! 
Those are just what I had in mind. :)

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[Noah Davis]

ndavis added a comment.


  In D16905#360075 , @trickyricky26 
wrote:
  
  > So should I include the renaming of the existing icons and adding of the 
ascending and descending versions of these new ones in this revision? That way 
applications that use `view-sort-ascending` for alphabetical order will have it 
replaced, but I guess that's okay since in that case the applications weren't 
using the icon properly in the first place.
  
  
  I don't see it hurting anything. I'd guess some apps are using it correctly 
and some aren't, but your new icons will still have a useful meaning that 
matches the situation.
  
  > The old `view-sort-ascending` and `descending` icons are also available in 
24px and 32px. If I rename those as well for consistency, will I need to add 
the new ones as `view-sort-ascending` and `descending`? Otherwise applications 
using `view-sort-ascending` will not be able to display a fitting icon.
  
  Yes, I would say you should also match sizes for the already existing icons. 
24px is the same as 22px, but with slightly wider margins.
  
  > Also should I change the fallback colors of the old SVG's to Shade Black as 
per the upcoming HIG change?
  
  I suppose so. It's hard to say because doing piecemeal changes to improve 
consistency also leads to more temporary inconsistency, but we have plenty of 
time before KF5.53.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16913: Add the possibility to give focus to the KPluginSelector search field

[Thomas Surrel]

thsurrel added a dependent revision: D16914: [Runners KCM] Focus the search 
field by default.

REPOSITORY
  R295 KCMUtils

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

To: thsurrel, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16913: Add the possibility to give focus to the KPluginSelector search field

[Thomas Surrel]

thsurrel created this revision.
thsurrel added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
thsurrel requested review of this revision.

REPOSITORY
  R295 KCMUtils

BRANCH
  arc_focussearch (branched from master)

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

AFFECTED FILES
  src/kpluginselector.cpp
  src/kpluginselector.h

To: thsurrel, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16751: Various grammar and capitalization fixes for UI consistency

[Luigi Toscano]

ltoscano added a comment.


  Please see the comments in D16749  and 
D16750 , many of them apply also here.

REPOSITORY
  R241 KIO

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

To: snoordhuis, #vdg, #localization
Cc: ltoscano, kde-frameworks-devel, michaelh, ngraham, bruns

D16905: Add a generic sorting options icon

[TrickyRicky]

trickyricky26 added a comment.


  So should I include the renaming of the existing icons and add ascending and 
descending versions of these new ones in this revision? That way applications 
that use `view-sort-ascending` for alphabetical order will have it replaced, 
but I guess that's okay because then the applications weren't using the icon 
properly in the first place.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[Noah Davis]

ndavis added a comment.


  In D16905#360055 , @trickyricky26 
wrote:
  
  > I have adjusted the arrows to fit the `view-sort-ascending` style:
  >
  > F6425856: sorting-icon-22-alt.png  
22px
  >  F6425879: sorting-icon-16-alt.png  
16px
  
  
  Very nice! I like these a lot.
  
  > However, I also noticed these can easily be changed to show ascending / 
descending order:
  >  F6425874: sorting-icon-22-alt-ascending.png 
 22px
  >  F6425883: sorting-icon-16-alt-ascending.png 
 16px
  > 
  > Maybe the new ascending and descending icons could replace the existing 
`view-sort-ascending` and `view-sort-descending`, while the sorting icons with 
alphabetic characters would be renamed to `view-sort-name-ascending` and 
`view-sort-name-descending`, as they represent sorting by name rather than 
generic sorting.
  
  `view-sort-ascending-name` and `view-sort-descending-name` would be better 
for the name specific stuff. This way if our software is using 
`view-sort-ascending-name` and a user switches their theme to something that 
doesn't have `view-sort-ascending-name`, they will get `view-sort-ascending` 
instead.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[TrickyRicky]

trickyricky26 planned changes to this revision.
trickyricky26 added a comment.


  I have adjusted the arrows to fit the `view-sort-ascending` style:
  
  F6425856: sorting-icon-22-alt.png  22px
  F6425879: sorting-icon-16-alt.png  16px
  
  In the next Revision update will rename them to `view-sort`.
  I think this looks a lot nicer. However, I also noticed these can easily be 
changed to show ascending / descending order:
  F6425874: sorting-icon-22-alt-ascending.png 
 22px
  F6425883: sorting-icon-16-alt-ascending.png 
 16px
  
  Maybe the new ascending and descending icons could replace the existing 
`view-sort-ascending` and `view-sort-ascending`, while the sorting icons with 
alphabetic characters would be renamed to `view-sort-name-descending` and 
`view-sort-name-descending`, as they represent sorting by name rather than 
generic sorting.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[Nathaniel Graham]

ngraham added a comment.


  In D16905#360028 , @ndavis wrote:
  
  > You mean sort of like the arrow on `view-sort-ascending` but going both 
ways? F6425832: Screenshot_20181115_143752.png 

  
  
  Exactly!

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[Noah Davis]

ndavis added a comment.


  In D16905#360021 , @ngraham wrote:
  
  > I was imagining vertical lines, not horizontal. A bit more like this 
(WARNING: exceptionally crude mockup approaching)
  >
  > F6425824: sorting-icon-22.png 
  
  
  You mean sort of like the arrow on `view-sort-ascending` but going both ways? 
F6425832: Screenshot_20181115_143752.png 

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[Nathaniel Graham]

ngraham added a comment.


  I was imagining vertical lines, not horizontal. A bit more like this 
(WARNING: exceptionally crude mockup approaching)
  
  F6425824: sorting-icon-22.png 

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[René J . V . Bertin]

rjvbb abandoned this revision.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16905: Add a generic sorting options icon

[Noah Davis]

ndavis added a comment.


  I think your original design looked the best. I don't think having lines 
extend to the area between the arrows makes the meaning significantly clearer.

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[René J . V . Bertin]

rjvbb added a comment.


  Friedrich W. H. Kossebau wrote on 20181115::17:41:58 re: "D16882 
<https://phabricator.kde.org/D16882>: [KDevelop/Shell] prevent duplicate added 
contextmenu actions"
  
  >   Please tell, are you not serious about using the CTags plugin and 
maintaining the integration?
  
  I cannot tell for sure but it's not something I'll be using every other 
minute. I am NOT planning to bring it into KDevelop, though; ideally those Kate 
plugins that make sense in KDevelop would work without having to patch anything 
outside of those plugins. I'll maintain the code in Kate as long as that 
remains doable.
  
  >   I need to know why I should spent my time on reviewing this patch and the 
related issues.
  
  Now that I have identified the upstream issue I will be abandoning this patch.
  
  >   Would you agree that it is surprising to have more than 1 signal per 
case? So would we agree that this is implicitly given?
  
  Yes, and probably, and I don't see a use-case for doing things another way 
but that doesn't exclude the possibility I'm overlooking one.
  
  >   What do you mean by platform dependent?
  
  UI events are, by definition.
  
  >   This bug became now also your bug 
  
  No. My problem, maybe, but not my bug.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16905: Add a generic sorting options icon

[TrickyRicky]

trickyricky26 added a comment.


  I do agree that the 22px version is clearer with two lines in between the 
arrows:
  
  F6425790: sorting-icon-22-alt.png 
  
  To fit this into the 16px was difficult, however:
  
  F6425793: sorting-icon-16-alt.png 
  
  I think the arrows are not clear enough at 100% scaling and the 2px gap in 
the middle looks awkward.

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[TrickyRicky]

trickyricky26 edited the summary of this revision.

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[Nathaniel Graham]

ngraham added a comment.


  Generally that was what I was looking for, thanks! I wonder how it would look 
with a vertical line or two between the arrows, though. Might make the concept 
a bit clearer?

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[Noah Davis]

ndavis added subscribers: ngraham, ndavis.
ndavis accepted this revision as: ndavis.
ndavis added a comment.
This revision is now accepted and ready to land.


  It looks good to me, but since @ngraham reported bug #393318, I'd like his 
opinion as well. Also, put this at the bottom of your summary:
  
  BUG: 393318
  FIXED-IN: 5.53
  
  What it does is automatically close the bug as resolved and say what version 
it will be fixed in when the patch gets landed.

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

To: trickyricky26, #vdg, #breeze, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add a generic sorting options icon

[TrickyRicky]

trickyricky26 edited the summary of this revision.
trickyricky26 edited the test plan for this revision.
trickyricky26 added reviewers: VDG, Breeze.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, #breeze
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16905: Add a generic sorting options icon

[TrickyRicky]

trickyricky26 created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
trickyricky26 requested review of this revision.

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

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

AFFECTED FILES
  icons-dark/actions/16/sort.svg
  icons-dark/actions/22/sort.svg
  icons/actions/16/sort.svg
  icons/actions/22/sort.svg

To: trickyricky26
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#359888 , @rjvbb wrote:
  
  > >   _If_ it is found that the root bug is in KTextEditor, sure.
  
  
  
  
  > See https://bugs.kde.org/show_bug.cgi?id=401069#c2
  
  That looks like good work onto finding the culprit code, Please concentrate 
the related discussion there.
  
  > I don't want to delve any deeper than that into code that isn't mine and 
I'm not planning to work otherwise. This goes beyond using the CTags plugin in 
KDevelop (which is someone else's idea) and as far as that use is relevant to 
me I'm perfectly happy with a workaround (here or in KTextEditor).
  
  Please check what you wrote here. To me it tells that you are in the wrong 
place.  KDevelop, KTextEditor & Co are not for mainly having fun tinkering with 
code of an IDE, but to create a usable product, which can be easily maintained 
in snippets of free time and which is independent from one single company's 
interest.
  
  Dumping hacked-together-until-it-works solutions one does not plan to work on 
further right from the start or really care for would be quickly the death of 
this.
  
  Please tell, are you not serious about using the CTags plugin and maintaining 
the integration? I need to know why I should spent my time on reviewing this 
patch and the related issues.
  
  >>   I can only urge you to invest into learning how to do debugging the 
stuff that you work on. It's basic developer tooling. 
  > 
  > Oh, I'm pretty confident I've logged more hours in more different debuggers 
than you, more than enough to know my strengths and weaknesses.
  
  Well, I just picked up your "I suck at debugging event-driven code". If that 
is your weakness, exercise on it to improve it. It's a required skill here, 
given that lots of stuff is event-driven in kdevelop. I do not make assumptions 
about what you have done and what not (and I also resist, almost, to hint to 
quantity vs, quality ;) ).
  
  >>   >   And here the API seems to be the emission of the 
KTextEditor::View::contextMenuAboutToShow signal, that one should only be done 
once and for the given view where the menu is shown on: "Signal which is 
emitted immediately prior to showing the current context menu". 
  > 
  > That doesn't say explicitly that there will only be 1 such signal for the 
active view so this aspect could even be platform dependent.
  
  Would you agree that it is surprising to have more than 1 signal per case? So 
would we agree that this is implicitly given? What do you mean by platform 
dependent?
  
  >> generation.s, but seeing myself still part of the near future kdevelop 
generation, here my banner script: "Stop creating code to work around bug 
symptoms ."
  > 
  > Another banner would "nobody is paid to fix someone else's bugs" (except 
for a few poor sods whom I hope get paid really well for it).
  
  This bug became now also your bug as it is inhibiting your desire to enable 
the ctags plugin. Bad luck, but also normal developer life.
  
  Your work-around  would become also my work-around as kdevelop 
co-contributor, and I do not like work-arounds when they are toll code for 
being lazy now. Even more if it is someone else being lazy.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[René J . V . Bertin]

rjvbb added a comment.


  >   _If_ it is found that the root bug is in KTextEditor, sure.
  
  Each KTextEditor::ViewPrivate has a KateViewInternal instance that inherits 
QWidget and overrides the contextMenuEvent() method. In that override it obains 
a QMenu from ViewPrivate::contextMenu() which is where KXMLGUI comes into play 
and where some disconnect/connect voodoo happens (reconnecting 2 signals from 
the same emitter to the same receiver, including the aboutToShowContextMenu 
slot). It then calls that menu's popup() method which triggers 
ViewPrivate::aboutToShowContextMenu() which in turn emits 
aboutToShowContextMenu.
  
  The question is thus probably if (and why) 
KateViewInternal::contextMenuEvent() is called for invisible QWidgets. If that 
doesn't happen the disconnect/connect voodoo in ViewPrivate::contextMenu() is 
probably to blame. I remember scratching my head about that bit in the past, 
shouldn't the disconnect be from *all* receivers?.
  See https://bugs.kde.org/show_bug.cgi?id=401069#c2
  
  I don't want to delve any deeper than that into code that isn't mine and I'm 
not planning to work otherwise. This goes beyond using the CTags plugin in 
KDevelop (which is someone else's idea) and as far as that use is relevant to 
me I'm perfectly happy with a workaround (here or in KTextEditor).
  
  >   I can only urge you to invest into learning how to do debugging the stuff 
that you work on. It's basic developer tooling. 
  
  Oh, I'm pretty confident I've logged more hours in more different debuggers 
than you, more than enough to know my strengths and weaknesses.
  
  >   >   And here the API seems to be the emission of the 
KTextEditor::View::contextMenuAboutToShow signal, that one should only be done 
once and for the given view where the menu is shown on: "Signal which is 
emitted immediately prior to showing the current context menu". 
  
  That doesn't say explicitly that there will only be 1 such signal for the 
active view so this aspect could even be platform dependent.
  
  > generation.s, but seeing myself still part of the near future kdevelop 
generation, here my banner script: "Stop creating code to work around bug 
symptoms ."
  
  Another banner would "nobody is paid to fix someone else's bugs" (except for 
a few poor sods whom I hope get paid really well for it).

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16878: Resolve symlinks in exclude folders

[Stefan Brüns]

bruns added a comment.


  Because it can never be consistent.
  What happens when I create two symlinks to the same folder, and put one link 
into includeFolders, the other one in excludeFolders?
  
  What really should happen, the indexer should **never** follow symlinks, but 
only add files by their canonical path. This avoids a bunch of problems, 
symlink loops, nondeterministic pathes for files when these are added to the 
index, ...

REPOSITORY
  R293 Baloo

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

To: poboiko, #frameworks, #baloo
Cc: bruns, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D16498: [KFileMetaData] Add extractor for DSC conforming (Encapsulated) Postscript

[Stefan Brüns]

bruns added a comment.


  In D16498#359676 , @poboiko wrote:
  
  > It seems like you've pushed something that was not intended to be pushed 
(XML extractor parts)
  
  
  No. Phabricator is just stupid.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, #baloo, astippich, ngraham, poboiko
Cc: pino, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#359595 , @rjvbb wrote:
  
  > >   Please, let's find the root causes and fix things at the base instead 
of adding such
  >
  > "we"
  
  
  (tried to not make this a "you" vs. "others" thing in the language, but yes, 
your itch only so far here, you have to scratch as a matter of normal software 
developer life)
  
  > find a fixable bug in some framework there's still no guarantee that no one 
will ever run KDevelop against a non-fixed version of that framework.
  
  _If_ it is found that the root bug is in KTextEditor, sure.
  
  For now the root is unknown, and there is some chance that it is actually the 
ctag plugin code which does something hacky to trigger the reported unexpected 
multiple parallel emissions of the menuAboutToShow signal.
  
  > I'll see if a breakpoint in and backtrace from my added if teaches me 
anything but I have my doubts (and no other ideas).
  
  I can only urge you to invest into learning how to do debugging the stuff 
that you work on. It's basic developer tooling. Like a serioius electrical 
engineer is expected to know how to use a multimeter or oscilloscope for the 
circuits they are fiddling with.
  
  >>   (even uncommented=unexplained=surprising)
  > 
  > That's easy to fix. See the "todo" comment elsewhere in the same file.
  >  TBH, I didn't add a comment yet because in itself I see nothing wrong in 
checking if you're dealing with the active view before you start adding things 
to a contextmenu.
  
  There are lots of things to check usually. But ones does not as one relies on 
callees fulfilling the explicit and implicit API contracts. And here the API 
seems to be the emission of the KTextEditor::View::contextMenuAboutToShow 
signal, that one should only be done once and for the given view where the menu 
is shown on: "Signal which is emitted immediately prior to showing the current 
context menu". 
  Adding guard code (thus complexity and stuff to maintain) to protect against 
contract breakages is only done as last resort.
  KTextEditor and kate ctags plugin are OpenSource. They can (and should) be 
fixed. Everything else is just adding debts for future code maintainers. Surely 
many humans are fine with having their fun & easy life now on the costs of 
future generation.s, but seeing myself still part of the near future kdevelop 
generation, here my banner script: "Stop creating code to work around bug 
symptoms ."

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[René J . V . Bertin]

rjvbb added a comment.


  So multiple contextMenu signals arrive in Kate too except they don't have any 
visible consequence.
  Let's see what the KTextEditor devs have to say about this. I'd rather stay 
away from getting too familiar with that framework, KXMLGUI even more.
  
  https://bugs.kde.org/show_bug.cgi?id=401069

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16894: [ECM] use a macro to test compiler flag support

[René J . V . Bertin]

rjvbb added inline comments.

INLINE COMMENTS

> apol wrote in KDECompilerSettings.cmake:195
> Why can't we just use check_cxx_compiler_flag?

We could of course. I worked under the assumption that is not done currently 
because checking only ID+version is faster. CMake's check_cxx_compiler_flag is 
more expensive because it invokes the compiler.

And it can fail, for instance if the user adds an option for which the compiler 
emits diagnostics output. That will be true for the "clang on Mac" situation 
too, but why generalise brittleness if you don't have to?

> apol wrote in KDECompilerSettings.cmake:203
> Why have a macro that only works on Apple?

? This is a macro that encapsulates a Mac-specific peculiarity but otherwise 
should work everywhere. Did I miss something?!

NB: `${_RESULT}` is not going to be set if `COMPILER_OK` isn't set first, 
meaning the macro should always "return false" when a flag isn't supported, 
even if check_cxx_compiler_flag isn't used.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system
Cc: apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, 
ngraham, bruns

D16894: [ECM] use a macro to test compiler flag support

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> KDECompilerSettings.cmake:195
> +# CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND CMAKE_CXX_COMPILER_VERSION 
> VERSION_LESS 6.0.0
> +macro (KDE_CHECK_CXX_COMPILER_FLAG _RESULT _FLAG)
> +# treat the arguments following _FLAG as a boolean expression

Why can't we just use check_cxx_compiler_flag?

> KDECompilerSettings.cmake:203
> +if (COMPILER_OK)
> +if(APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")
> +# Clang on APPLE needs verification because of Apple's

Why have a macro that only works on Apple?

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system
Cc: apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, 
ngraham, bruns

D16894: [ECM] use a macro to test compiler flag support

[René J . V . Bertin]

rjvbb created this revision.
rjvbb added a reviewer: Build System.
rjvbb added a project: Build System.
Herald added a project: Frameworks.
Herald added a subscriber: kde-buildsystem.
rjvbb requested review of this revision.

REVISION SUMMARY
  It is crucial not to add unsupported compiler options because they can cause 
CMake's `check__compiler_flags` macro to fail, causing problems like 
https://bugs.kde.org/show_bug.cgi?id=401018.
  To prevent this, run an actual test when using clang on Mac.
  
  This is almost unavoidable because determining compiler flag availability 
from compiler ID + version can be tricky to near impossible on Mac. Apple's 
llvm version numbers that are ahead of stock llvm versions and not easy to 
match. In addition, not all cmake versions distinguish between stock clang and 
"AppleClang" and those that do need policy `CMP0025` set to `NEW`. That policy 
cannot be set after the `project` statement in the toplevel cmake file (= not 
in an ECM module).
  
  This patch introduces a set of macros to simplify writing reliable 
conditional code for adding compiler flags. If will do the usual compiler ID + 
version checking to determine if a flag should be supported; when using Clang 
on Mac it will in addition use `check__compiler_flag()`. This should be 
safe because Apple's llvm versions have always been ahead of stock versions; 
false negatives should thus never occur.
  The C++ version of the new macro is used where possible.
  
  BUG: 401018

TEST PLAN
  See https://bugs.kde.org/show_bug.cgi?id=401018 ; without this patch the 
project fails to build using Xcode compiler versions because the generated 
kcddb_export.h header does not match the visibility settings.

REPOSITORY
  R240 Extra CMake Modules

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake

To: rjvbb, #build_system
Cc: kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, ngraham, 
bruns

D16880: Update PlasmaWindowModel test to reflect VirtualDesktop changes

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:f1c8b16a0026: Update PlasmaWindowModel test to reflect 
VirtualDesktop changes (authored by davidedmundson).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16880?vs=45463=45511

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

AFFECTED FILES
  autotests/client/test_plasma_window_model.cpp

To: davidedmundson, #kwin, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16883: Add VirtualDesktops to PlasmaWindowModel

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:98b0ad34b68f: Add VirtualDesktops to PlasmaWindowModel 
(authored by davidedmundson).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16883?vs=45483=45512

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

AFFECTED FILES
  autotests/client/test_plasma_virtual_desktop.cpp
  autotests/client/test_plasma_window_model.cpp
  src/client/plasmawindowmanagement.cpp
  src/client/plasmawindowmodel.cpp
  src/client/plasmawindowmodel.h
  src/server/plasmawindowmanagement_interface.cpp

To: davidedmundson, #kwin, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16877: Cleanup windowInterface in tests before windowManagement is destroyed

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:06c18f5869f3: Cleanup windowInterface in tests before 
windowManagement is destroyed (authored by davidedmundson).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16877?vs=45455=45510

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

AFFECTED FILES
  autotests/client/test_plasma_virtual_desktop.cpp

To: davidedmundson, #kwin, mart
Cc: mart, kde-frameworks-devel, michaelh, ngraham, bruns

D16883: Add VirtualDesktops to PlasmaWindowModel

[Marco Martin]

mart accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #kwin, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16880: Update PlasmaWindowModel test to reflect VirtualDesktop changes

[Marco Martin]

mart accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #kwin, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16877: Cleanup windowInterface in tests before windowManagement is destroyed

[Marco Martin]

mart accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #kwin, mart
Cc: mart, kde-frameworks-devel, michaelh, ngraham, bruns

D16885: Delete the correct item in removeDesktop

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:f25eadc92c47: Delete the correct item in removeDesktop 
(authored by davidedmundson).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16885?vs=45488=45504

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

AFFECTED FILES
  src/server/plasmavirtualdesktop_interface.cpp

To: davidedmundson, #kwin, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16878: Resolve symlinks in exclude folders

[Igor Poboiko]

poboiko added a comment.


  In D16878#359442 , @bruns wrote:
  
  > IMHO we should just disallow specifying symlinks in both 
include/excludeFolders. The user can just use exludeFolders = /storage/stuff if 
he wants to exclude it.
  
  
  Why not? This kind of (partial) symlink support is quite easy to implement. 
And the more restrictions users have, the worse their experience is.
  (I've just stumbled upon this issue randomly, when added some folder - not 
even thinking at that time that it is a symlink - and, after some time, found 
out that files there were indexed and my exclusion rule was silently ignored)

REPOSITORY
  R293 Baloo

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

To: poboiko, #frameworks, #baloo
Cc: bruns, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D16876: [balooctl] Add possibility to create a copy of the index without freelist

[Igor Poboiko]

poboiko added a comment.


  In D16876#359454 , @bruns wrote:
  
  > You are replicating `mdb_copy -c` here.
  >  [...]
  
  
  Yep, I know. It's just there is no proper documentation for this issue (well, 
this applies for the whole baloo - T7843: Documentation 
), and so I just thought users are more 
likely to discover it if it would such a feature in `balooctl` (which people 
use sometimes, hopefully).

REPOSITORY
  R293 Baloo

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

To: poboiko, #frameworks, #baloo
Cc: bruns, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D16498: [KFileMetaData] Add extractor for DSC conforming (Encapsulated) Postscript

[Igor Poboiko]

poboiko added a comment.


  It seems like you've pushed something related to XML extractor as well

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, #baloo, astippich, ngraham, poboiko
Cc: pino, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams