Fix disappearing searchbar when bookmark menu is opened for another tab.
ClosedPublic

Authored by martinkostolny on Mar 26 2018, 10:29 PM.

Details

Summary

When menu is cleaned / removed _quickSearchAction is probably removed with it. This patch prevents it by removing the action before menu recreation.

Test Plan
  1. open bookmark popup and check that quickSearch bar is there
  2. click on bookmarks icon on another panel, check that quickSearch bar is still there

Diff Detail

Repository
R167 Krusader
Branch
bookmark-quicksearch-disappearing-searchbar
Lint
No Linters Available
Unit
No Unit Test Coverage
Restricted Application added a subscriber: kde-doc-english. · View Herald TranscriptMar 26 2018, 10:29 PM
martinkostolny requested review of this revision.Mar 26 2018, 10:29 PM
martinkostolny retitled this revision from Summary: Fix disappearing searchbar when bookmark menu is opened for another tab. to Fix disappearing searchbar when bookmark menu is opened for another tab..
martinkostolny edited the test plan for this revision. (Show Details)
martinkostolny added a project: Krusader.

Sorry for the first iteration in this CR. It seems I still have much to learn with arc :).

nmel accepted this revision.Mar 27 2018, 6:02 AM

Works nicely, thanks!

When menu is cleaned / removed _quickSearchAction is probably removed with it.

I think the root cause is slightly different because if your statement were true, the bar wouldn't be shown again with the same menu where it was shown for the first time. Likely, if you add an action to many menus (in this case, each tab has its own menu), QWidgetAction is displayed only on the first one. Given many menus could be visible at the same time, it might be a feature, not a bug...

This revision is now accepted and ready to land.Mar 27 2018, 6:02 AM
This revision was automatically updated to reflect the committed changes.

I think the root cause is slightly different because if your statement were true, the bar wouldn't be shown again with the same menu where it was shown for the first time. Likely, if you add an action to many menus (in this case, each tab has its own menu), QWidgetAction is displayed only on the first one. Given many menus could be visible at the same time, it might be a feature, not a bug...

Your explanation seems to make more sense :). Thanks for testing!

rade added a comment.Mar 27 2018, 9:54 PM

This issue was closed just as I was commenting... Might be too late now but I just wanted to add that it might be a good idea to add a comment explaining the reason why we are removing quickssearchaction from the old menu as that is not really obvious by just reading the code.

nmel added a comment.Mar 28 2018, 7:04 AM

I agree with Rade.

Sorry about that, I should have waited a bit longer. So how about this?

// removing action from previous menu is necessary 
// otherwise it wouldn't be displayed in currently populating menu
if (_mainBookmarkPopup) {
    _mainBookmarkPopup->removeAction(_quickSearchAction);
}
nmel added a comment.Mar 29 2018, 7:30 AM

Looks good.

rade added a comment.EditedMar 29 2018, 7:10 PM
// removing action from previous menu is necessary 
// otherwise it wouldn't be displayed in currently populating menu

Looks good, although I think it should be "won't" not "wouldn't" as it is an action that will not happen in the future not an action that has not happened in the past. I think there should be a 'the' between 'in' and 'currently' as well, but it's perfectly understandable without it.

Looks good, although I think it should be "won't" not "wouldn't" as it is an action that will not happen in the future not an action that has not happened in the past. I think there should be a 'the' between 'in' and 'currently' as well, but it's perfectly understandable without it.

I've pushed the comment with your points included. Thanks! :)