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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#494867 , @rjvbb wrote:
  
  > I could try your solution, of course, but what annoys me is that it comes 
months after I worked on mine.
  
  
  I think we all share the feeling that this should have already been handled 
the first time completely to end and now be a thing of the past, given we all 
once spent time on it before and would prefer not to have spent more :)
  
  > It would help if you had a specific critique on my solution other than "it 
doesn't use this or that signal" (or, what I kind of sense, "it comes from 
you").
  
  I made some respective comment in D22424#494708 
 while you asked.
  
  To emphasize once more, I was about to give your proposed patch a go as it 
worked by what I tested, but trying to understand the problem fixed with this 
patch with proper depth to give you a proper review, I played around and then 
found that the approach one would have done initially (as everyone seems to 
have tried) still might work out, if one simply connects to the abouttohide 
signal of the currently passed context menu, instead of assuming a static one 
per view. The current clean-only-on-next-show code always has felt rather messy 
to me. And so I simply proposed what arose by accident from my experiments 
around the phenomenon as an alternative solution, to compare & perhaps inspire 
for more elegant solution.
  
  > No disrespect intended, but your description in D22424 
 isn't that easy to read either (it felt 
like reading German, for some inexplicable reason ;) ).
  
  Yes, rereading I see how this was rather a memory dump trying to still get 
something written after having spent some hours on it, before falling into the 
bed. Will see to update a bit.
  
  > You claim that
  > 
  >> For some yet to explore internal reason, if there is a plugin with a
  >>  "ktexteditor_popup" menu to merge in, then the QMenu object instance is 
the
  >>  same across all KTextEditor::View instances, otherwise each view has its
  >>  own.
  > 
  > but I have never seen any proof that the context QMenu instance is ever NOT 
the same. On the contrary, it makes perfect sense that it is always the same 
because there can only be a single context menu.
  
  I saw this from all the debug output I had added, comparing pointers of the 
context menu object handed in. Ideally indeed I would have also spent more time 
in kxmlgui code to find the code culprit for this to reason why this happens, 
but my test log showed consistent results for the different settings (ctags 
plugin enabled, not enabled)., so I stayed with just the observed behaviour 
from that black box kxmlgui, as it showed what cases one has to deal with in 
released versions of kxmlgui.
  
  > I'm not saying we shouldn't rely on the aboutToHide signal if this signal 
can indeed be relied on. But it would be good to do things properly and get to 
the bottom of the shared-or-not context QMenu instance question. It looks like 
your new `QPointer` is an acknowledgement to the fact that it can at 
least be a unique instance; if it always is it could (and maybe should) be 
moved to a central, shared structure and accessed from there.
  
  The idea behind the `QPointer` member is rather to be able to have 
access to any last added-to context menu to remove any persistent actions from 
it (those not created on-the-fly but owned by plugins and only shared for the 
context menu), in the very case any action triggered from the context menu 
results in the deletion of the textdocument object while the menu is still open
  
  > Maybe the KTextEditor framework should simply provide a method to get a 
pointer to the current context menu?!
  
  The problem is at least that the API of KTextEditor::View allows to set a 
custom menu per view instance. If we go with the assumption that only one 
context menu can be active globally at the same time (which might be true in 
case of popup menus at least), KTextEditor could perhaps help and track this 
internally and expose access.
  For our use-case though reliable "abouttoshow" & "abouttohide" signals (with 
ktexteditor ensuring reliability by all means) might be more helpful, so we do 
not have to do all the extra tracking work on our side.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau, mwolff
Cc: mwolff, egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, 
hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, 
geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

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

[René J . V . Bertin]

rjvbb added a comment.


  I could try your solution, of course, but what annoys me is that it comes 
months after I worked on mine. I currently have too many other things going on 
in my life to dive in and figure out what on earth was going on again. I do 
think I outlined it well enough above in the initial description and/or 
exchange; I should find a moment this weekend to sit down and re-read it with a 
fresh mind.
  It would help if you had a specific critique on my solution other than "it 
doesn't use this or that signal" (or, what I kind of sense, "it comes from 
you"). No disrespect intended, but your description in D22424 
 isn't that easy to read either (it felt 
like reading German, for some inexplicable reason ;) ).
  
  You claim that
  
  > For some yet to explore internal reason, if there is a plugin with a
  >  "ktexteditor_popup" menu to merge in, then the QMenu object instance is the
  >  same across all KTextEditor::View instances, otherwise each view has its
  >  own.
  
  but I have never seen any proof that the context QMenu instance is ever NOT 
the same. On the contrary, it makes perfect sense that it is always the same 
because there can only be a single context menu.
  
  I'm not saying we shouldn't rely on the aboutToHide signal if this signal can 
indeed be relied on. But it would be good to do things properly and get to the 
bottom of the shared-or-not context QMenu instance question. It looks like your 
new `QPointer` is an acknowledgement to the fact that it can at least be 
a unique instance; if it always is it could (and maybe should) be moved to a 
central, shared structure and accessed from there.
  Maybe the KTextEditor framework should simply provide a method to get a 
pointer to the current context menu?!

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau, mwolff
Cc: mwolff, egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, 
hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, 
geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#494702 , @rjvbb wrote:
  
  > Please check the earlier discussion; IIRC there is a reliability problem 
with that signal, and I did try reverting to its use before coming up with the 
current solution.
  
  
  I read indeed the very discussion before, a few times even (see also that I 
fixed the link to the blog post in the commit message of mine) :)
  Initially I hoped we could just go with your patch and be done, but I wanted 
to understand the problem in details first, to not have to rely on IIRC and 
vague commit messages and historic situations .And when doing so I found 
nothing which actually prevent to use that signal, but rather some issue in the 
original commit which tried to use it. So please take some time to read the 
text about that patch to understand why I rather went to propose that as 
altenativenow.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau, mwolff
Cc: mwolff, egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, 
hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, 
geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

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

[René J . V . Bertin]

rjvbb added a comment.


  Please check the earlier discussion; IIRC there is a reliability problem with 
that signal, and I did try reverting to its use before coming up with the 
current solution.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau, mwolff
Cc: mwolff, egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, 
hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, 
geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  While trying to understand the problem, I found that perhaps we could still 
revert to using the `aboutToHide` signal, with proper boundaroes.
  So alternative solution  proposal up as D22424 


REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau, mwolff
Cc: mwolff, egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, 
hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, 
geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

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

[René J . V . Bertin]

rjvbb added a comment.


  Milian Wolff wrote on 20190112::12:35:11 re: "D16882 
<https://phabricator.kde.org/D16882>: [KDevelop/Shell] prevent duplicate added 
contextmenu actions"
  
  >> mwolff wrote in textdocument.cpp:691
  >>  can't you just add a slot here that removes the actions we added once the 
menu is closed? that would fix this issue with way less code
  > 
  > and with slot I mean an local lambda that takes a copy of the actions list
  
  I agree that would be more elegant, and there was code (yours, IIRC) which 
aimed to do this. The problem is that for some reason the aboutToClose signal 
is not reliable.

REPOSITORY
  R32 KDevelop

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

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

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

[Milian Wolff]

mwolff added inline comments.

INLINE COMMENTS

> mwolff wrote in textdocument.cpp:691
> can't you just add a slot here that removes the actions we added once the 
> menu is closed? that would fix this issue with way less code

and with slot I mean an local lambda that takes a copy of the actions list

REPOSITORY
  R32 KDevelop

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

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

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

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> textdocument.cpp:691
> -menu->addAction(action);
> -}
> -}

can't you just add a slot here that removes the actions we added once the menu 
is closed? that would fix this issue with way less code

REPOSITORY
  R32 KDevelop

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

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

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

[René J . V . Bertin]

rjvbb added a comment.


  bump?

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 updated this revision to Diff 45797.
rjvbb added a comment.


  Well, apparently the contextmenu CAN change during a session (at least on Mac 
and when I open it in different opened-at-session-load documents when the 
initial project load and parsing activity is still in progress).
  
  Using a QPointer fixes that but I left in debugging traces for good 
measure.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16882?vs=45782=45797

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

AFFECTED FILES
  kdevplatform/shell/textdocument.cpp
  kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 set the repository for this revision to R32 KDevelop.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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.


  >   E.g. I would have expected before looking at things that each view has 
their own separate context menu instance, possibly even created on the fly per 
display :)
  
  I think that is what I would have expected too (maybe not per display :)), 
and that's one reason I didn't feel like delving into KXMLGUI. That kind of 
dynamic approach would surely have made things much more complex in that 
framework. Instead, it looks like that GUI "skeleton" is created once and 
reused, and that must of course be a lot simpler.
  This also implies that it must be evident for anyone knowning KXMLGUI that 
the context menu instance for `aboutToShowContextMenu` signals is shared among 
views. As to the using a custom context menu feature: I suppose that devs using 
that are expected to know what they're doing...
  
  FWIW, this is exactly why I've subscribed the frameworks ML to this diff. Do 
you think we need to make a more targeted effort to draw KTextEditor developer 
attention to these last few comments?

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 set the repository for this revision to R32 KDevelop.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 updated this revision to Diff 45782.
rjvbb added a comment.


  Another fix: use the active MainWindow as the parent of the contextMenuData 
instance and do NOT delete it in the `TextDocumentPrivate` dtor.
  
  Also, do not assume there will ever only be a single MainWindow in a KDevelop 
session: the least we can do is reset `TextDocumentPrivate::contextMenuData` 
when that instance is deleted.
  
  This should round up any issues with the new approach; I don't see anything 
that cries for immediate improval. Instead it'd be nice if this could make the 
5.3.1 release, being a bugfix.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16882?vs=45761=45782

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

AFFECTED FILES
  kdevplatform/shell/textdocument.cpp
  kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 updated this revision to Diff 45761.
rjvbb added a comment.


  Apologies, the tear-off bit shouldn't have been included of course.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16882?vs=45759=45761

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

AFFECTED FILES
  kdevplatform/shell/textdocument.cpp
  kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 updated this revision to Diff 45759.
rjvbb added a comment.


  updated as suggested:
  
  - moves the context menu added stuff logic into a dedicated class (as an 
upbeat to possible future improvement)
  - caches the QMenu* to which actions were last added, and removes them from 
that menu when the next context menu is shown. This should be equivalent to 
removing the items on `contextMenuAboutToHide`.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16882?vs=45642=45759

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

AFFECTED FILES
  kdevplatform/shell/textdocument.cpp
  kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 set the repository for this revision to R32 KDevelop.

REPOSITORY
  R32 KDevelop

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

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

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#361733 , @rjvbb wrote:
  
  > >   `KTextEditor::View::contextMenu()` talks about "the xmlgui menu" or 
custom set "context menu object", but no hint/promise whether there is any 
instance sharing done, e.g. between views for the same document or even across 
all views in the same process(?).
  >
  > No, but from the code it's clear that there's only a single instance that 
is shared among all views. With KXMLGUI that's even unavoidable (and 
incidentally a source of problems on Mac but that's a different can of worms).
  
  
  What I meant is: in a perfect world from KDevelop side when writing code 
against KTextEditor API we would only need to look as far as the API and its 
documentation. Anything which is not specified in the API dox is implementation 
details, and the KTextEditor developers would be free to change things as they 
need, unless they break a promise given in the API dox.
  
  Thus it would be nice to have this detail specified in the API dox of 
KTextEditor, about what to expect about the lifetime of the QMenu instance and 
if it is shared and if so, between what.
  That would help both sides, the developers of KTextEditor to know what they 
are bound to, as well as users of the API to know what to prepare for/deal with.
  
  E.g. I would have expected before looking at things that each view has their 
own separate context menu instance, possibly even created on the fly per 
display :)
  
  So, "from the code it's clear" ideally would be only interesting when trying 
to find bugs. When writing code, the API should be what we look at, and not 
further. Anything else is a bug/missing feature in the API.
  
  This is orthogonal to your actually proposed bug fix, which might be the 
final fix. But right now it would be relying on internal implementation, not 
what is documented in the API (by what I quickly read).
  (sorry, myself not enough time left now, more the next week if still needed)

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, 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 subscriber: egospodinova.
rjvbb added a comment.


  >   I would see the flaw also in that there is no specification in the 
KTextEditor API how the context menu is shared/reused.
  
  Did you see the reaction to my proposed KTextEditor fix? My argument "sounds 
reasonable". To me that suggests no one ever thought about whether there should 
only be a single signal, the one for the active view.
  
  >   `KTextEditor::View::contextMenu()` talks about "the xmlgui menu" or 
custom set "context menu object", but no hint/promise whether there is any 
instance sharing done, e.g. between views for the same document or even across 
all views in the same process(?).
  
  No, but from the code it's clear that there's only a single instance that is 
shared among all views. With KXMLGUI that's even unavoidable (and incidentally 
a source of problems on Mac but that's a different can of worms).
  
  >   That might be a way, yes.
  
  It's the simplest approach and the most robust, the poor man's equivalent of 
moving the whole context menu logic to KDevelop::MainWindow where the question 
of what view is active shouldn't arise (I didn't check if this is a feasible 
alternative). I'd call it elegantly simple if it weren't for the fact it 
involves a global static variable.
  
  As to the other alternative,
  
  >   Another option might be to link up to the menu being closed and clean up 
then, as the comment on the `addedContextMenu` member claimed. (personally 
preferred to clean up right after use). 
  
  my first reflex was to start implementing that, then I saw this could get 
complex and possibly ugly when I went over the various things the code would 
have to take into account. I didn't write them down (sorry) but I do remember 
being suspicious of Qt's aboutToHide signal, apparently with reason 
(http://labs.trolltech.com/blogs/2010/02/23/unpredictable-exec/).
  And then I realised only a tiny change was required ...
  
  A truly proper implementation would probably have a dedicated context menu 
class that has the addedContextMenu QMenu with KDevelop actions, changing that 
only when required instead of rebuilding it every time, etc. And that sounds 
more like a junior job than as a bugfix to me, esp. since it would apparently 
require thorough checking of the current `aboutToHide` reliability.
  
  A middle ground is possible: a dedicated context menu class that for now 
keeps the same working principle (JIT removal).
  
  >   Given the current implementation kdevelop-side I would not be surprised 
if KTextEditor changed implementation here, but needs to be explored.
  
  Don't hesitate to double-check, but I didn't see any recent changes in the 
code involved. The strange disconnect/reconnect trick (now fixed) has been 
there for years, in particular.
  
  >   The proposed fix relies on the current implementation, which is a bit 
fragile.
  
  ? It relies on the fact that the context menu is always the same instance, 
and that's a given as long as it's a kxmlgui menu, no? Removing non-existent 
menu actions doesn't (currently) generate errors, so there's no problem there. 
And we could cache the target QMenu address in order to raise a warning if ever 
we get a different one.
  
  Hah! Actually, the JIT implementation could remove d->addedContextMenu from 
the cached QMenu instance. That should make the approach work whether the menu 
is always the same or whether it changes all the time, because there will still 
ever be only a single context menu active at a time.

REPOSITORY
  R32 KDevelop

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

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

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  For the record, related commits are:
  Adding the principal clean-up logic:  
R32:b837392d5f05394794a813afb7ca94e54650fcff 

  Changing it from clean-on-hide to clean-on-about-to-show-again: 
R32:4d63d74c7d189479b752a11df70afab77859a457 


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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#360857 , @rjvbb wrote:
  
  > Re-opening because I found an actual flaw in KDevelop after noticing that 
context menu duplication still occurred when only the active view receives the 
aboutToShowContextMenu signal.
  >
  > The `addedContextMenu` member exists because `"we want to remove the added 
stuff when the menu hides"`. This should of course read "when the menu reopens 
in again, possibly in a different view".
  
  
  Good find, that seems indeed broken.
  
  > The flaw here is that the design forgets that the context menu instance is 
shared among views. It expects `d->addedContextMenu` to exist and contain the 
QMenu added by a previous view, but this cannot be the case in the current 
implementation where the variable is only allocated when the menu is first 
opened in a given view.
  
  I would see the flaw also in that there is no specification in the 
KTextEditor API how the context menu is shared/reused. 
`KTextEditor::View::contextMenu()` talks about "the xmlgui menu" or custom set 
"context menu object", but no hint/promise whether there is any instance 
sharing done, e.g. between views for the same document or even across all views 
in the same process(?).
  
  > If the `addedContextMenu` is to be removed in JIT-fashion before reopening 
the context menu, it should be a static variable.
  
  That might be a way, yes.
  
  Before though I would like to have it first sorted out with the KTextEditor 
people what to expect here and whether the API dox could resolve that 
undefinedness. Given the current implementation kdevelop-side I would not be 
surprised if KTextEditor changed implementation here, but needs to be explored. 
The proposed fix relies on the current implementation, which is a bit fragile.
  
  Another option might be to link up to the menu being closed and clean up 
then, as the comment on the `addedContextMenu` member claimed. (personally 
preferred to clean up right after use). 
  Also needs to be explored if there once was such an implementation and why it 
has been removed. Might do in the next days.

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.


  In case anyone wonders why this has gone undetected: I think because of an 
undocumented feature, the fact `aboutToShowContextMenu` was called for all 
views. Indeed, with the KTextEditor fix in place the duplication issue occurs 
also without loading the CTags plugin (= with stock KDevelop code).

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 edited the summary of this revision.
rjvbb edited the test plan for this revision.
rjvbb set the repository for this revision to R32 KDevelop.

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 updated this revision to Diff 45642.
rjvbb added a comment.


  New patch, same purpose, active principle as outlined in the reopening 
comment.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16882?vs=45471=45642

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

AFFECTED FILES
  kdevplatform/shell/textdocument.cpp

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 reclaimed this revision.
rjvbb added a comment.
This revision now requires changes to proceed.


  Re-opening because I found an actual flaw in KDevelop after noticing that 
context menu duplication still occurred when only the active view receives the 
aboutToShowContextMenu signal.
  
  The `addedContextMenu` member exists because `"we want to remove the added 
stuff when the menu hides"`. This should of course read "when the menu reopens 
in again, possibly in a different view".
  The flaw here is that the design forgets that the context menu instance is 
shared among views. It expects `d->addedContextMenu` to exist and contain the 
QMenu added by a previous view, but this cannot be the case in the current 
implementation where the variable is only allocated when the menu is first 
opened in a given view.
  If the `addedContextMenu` is to be removed in JIT-fashion before reopening 
the context menu, it should be a static variable.
  
  Fix upcoming.

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 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

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

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

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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   Please, let's find the root causes and fix things at the base instead of 
adding such
  
  I suck at debugging event-driven code, unfortunately. But even if "we" 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.
  
  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).
  
  >   (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.
  
  Ultimately the problem lies in having a single contextmenu (the `menu` 
argument) and a `d->addedContextMenu` per document (view). As I said on the ML, 
moving `populateContextMenu()` and `addedContextMenu` to a class like 
KDevelop::MainWindow would solve the issue too without having to rewrite 
`populateContextMenu()`. There can only be a single contextmenu active at any 
time, so it would make sense to populate it in a singleton class, no?

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

[Friedrich W. H. Kossebau]

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


  Oh, I missed the latter "For an as-yet unknown reason populateContextMenu() 
will be called with every view in which the contextmenu has been opened when 
the CTags plugin is loaded. A bug in KTextEditor maybe?"
  
  Please, let's find the root causes and fix things at the base instead of 
adding such (even uncommented=unexplained=surprising) work-arounds for 
symptoms. Saves future code readers/maintainers from wanting to toast the 
original developers of such confusing bug/misbehaviour masking code.

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  > TextDocument::populateContextMenu() is called when the user opens the 
contextmenu but this can happen for more than just the view currently active.
  
  What does "more than just the view currently active" mean? How can that 
happen? From what I see KTextEditor::View::contextMenuAboutToShow signal 
triggers the method. Why would there be multiple signals?

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop
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 created this revision.
rjvbb added a reviewer: KDevelop.
rjvbb added a project: KDevelop.
rjvbb requested review of this revision.

REVISION SUMMARY
  `TextDocument::populateContextMenu()` is called when the user opens the 
contextmenu but this can happen for more than just the view currently active. 
When that happens, each view will add its own copy of the list of items to be 
added to the contextmenu, resulting in duplicate entries.
  
  This patch prevents the consequence of calling `populateContextMenu()` 
multiple times by checking if the target view is indeed the one currently 
active in the mainWindow that it belongs to, *after* removing the items from 
`d->addedContextMenu` from the target `menu`.
  
  Preventing the multiple calls might also be possible but would probably 
introduce more complexity (connecting and disconnecting `populateContextMenu()` 
in reaction to `focusIn` and `focusOut` signals?).
  
  This issue can be triggered by loading the Kate CTags plugin (after applying 
D16779 ). For an as-yet unknown reason 
`populateContextMenu()` will be called with every view in which the contextmenu 
has been opened when the CTags plugin is loaded. A bug in KTextEditor maybe?

TEST PLAN
  1- apply D16779  to Kate, rebuild and 
reinstall the kate-ctags plugin
  2- load that plugin in KDevelop; observe contextmenu action duplication (once 
for each document that has opened a contextmenu)
  3- apply this patch, rebuild and reinstall libKDevPlatformShell
  4- repeat 2, contextmenu is free of duplicates

REPOSITORY
  R32 KDevelop

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

AFFECTED FILES
  kdevplatform/shell/textdocument.cpp

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