Community
Participate
Working Groups
The new "Hide" command on toolbar groups interferes with "Customize Perspective". It doesn't make sense to have yet another way to interact with toolbar items. When I e.g. hide the toolbar group with "New Java Package" and "New Java Class" and then open the "Customize Perspective" dialog, then I still see those two commands enabled in the "Java Element Creation" group. When I enable the "Java Project..." command, then it also doesn't show up in the UI. You can't add a UI action that kills existing functionality. Either make the "Hide" command toggle the enablement in the "Customize Perspective" dialog, or remove the whole feature again.
Furthermore the command name "Restore Hidden Toolbars Entries" is grammatically incorrect. I've removed this item from the N&N.
The "Restore Hidden Toolbars Entries" command is also a no-go. If a user accidentally removed one toolbar group, then the only way to revert that is to restore everything and then hide all other groups again. That doesn't scale.
(In reply to Markus Keller from comment #1) > Furthermore the command name "Restore Hidden Toolbars Entries" is > grammatically incorrect. > > I've removed this item from the N&N. What would be your desired text?
(In reply to Markus Keller from comment #2) > The "Restore Hidden Toolbars Entries" command is also a no-go. If a user > accidentally removed one toolbar group, then the only way to revert that is > to restore everything and then hide all other groups again. That doesn't > scale. Perhaps a better workflow for "Restore Hidden Toolbars" would be to pop open the CPD at page they can restore the toolbars from. PW
(In reply to Paul Webster from comment #4) > (In reply to Markus Keller from comment #2) > > The "Restore Hidden Toolbars Entries" command is also a no-go. If a user > > accidentally removed one toolbar group, then the only way to revert that is > > to restore everything and then hide all other groups again. That doesn't > > scale. > > Perhaps a better workflow for "Restore Hidden Toolbars" would be to pop open > the CPD at page they can restore the toolbars from. > > PW +1 if CPD would work, but the CPD dialog is broken since a few releases.
Then enough of it needs to be fixed to make this functionality work. At the very least it can't make CPD worse. If we can't bring it up to snuff then we might need to remove it and retarget it for Mars. PW
(In reply to Lars Vogel from comment #3) > What would be your desired text? E.g. "Restore Hidden Toolbar Groups" would have been correct. Just to state the obvious: This item was not removed from the N&N because of a bad name, but because the workflow is wrong and the feature breaks even more of the CPD. I agree with Paul that a better workflow would be to: - have "Customize Perspective..." back in the toolbar context menu (should pre-select the tool item on which the menu was opened) - offer a button "Restore Hidden Items" in the CPD That would allow the user to re-enable individual items, and the "Restore" button would even let the user judge the impact of that action before applying that potentially large-scale change.
Created attachment 242717 [details] Screenshot of the Outline view after hiding its toolbar. I see another three problems related to this feature: 1. It allows to hide the perspective spacer which is responsible for right alignment of quick access and perspective switcher. 2. When a view toolbar is hidden (e.g. in Outline view), the view menu button position is incorrect (see screenshot). It is corrected after resizing the view. 3. After hiding a view toolbar, there is no way to restore it (at least in some views).
And another issue caused by this: bug 432686.
*** Bug 432686 has been marked as a duplicate of this bug. ***
(In reply to Dani Megert from comment #10) > *** Bug 432686 has been marked as a duplicate of this bug. *** Nope, that has nothing to do with this development.
(In reply to Markus Keller from comment #7) > (In reply to Lars Vogel from comment #3) > > What would be your desired text? > > E.g. "Restore Hidden Toolbar Groups" would have been correct. It is not only Toolbar Groups (visible in the CPD) it allows also to hide ToolControls, e.g. the QuickAccess box (currently not available in the CDP), I think "Restore Hidden Toolbar Entries" is the correct term. I opened Bug 434198 for this change.
I vote -1 for a removal of this functionality in Luna. The ability to hide the QuickAccess box was one of the bugs most users complaint about. See Bug 362420 which has 64 registered users. If other platform committers would like to get this removed, I potentially should adjust my voting but just saying we need to integrate a broken functionality or remove it, is IMHO not the right approach. I'm +2 for improving that in Mars and integrating that into CDP. We have Bug 432075 for that.
(In reply to Lars Vogel from comment #11) > (In reply to Dani Megert from comment #10) > > *** Bug 432686 has been marked as a duplicate of this bug. *** > > Nope, that has nothing to do with this development. Well, it was you who claimed this in bug 432686 comment 3: "Ah sorry, I assume this was my change to hide toolbar entries. Looks like this is valid for the whole hidding of the toolbar which I have not touched."
(In reply to Dani Megert from comment #14) > Well, it was you who claimed this in bug 432686 comment 3: > "Ah sorry, I assume this was my change to hide toolbar entries. Looks like > this is valid for the whole hidding of the toolbar which I have not touched." Sorry for not updating the bug after my investigation. In my test I reverted the "Hide toolbar" changes and the issue of Bug 432686 was still present.
(In reply to Wojciech Sudol from comment #8) > 3. After hiding a view toolbar, there is no way to restore it (at least in > some views). This is really a blocker and is enough to justify removal of this feature. (In reply to Lars Vogel from comment #13) > I vote -1 for a removal of this functionality in Luna. The ability to hide > the QuickAccess box was one of the bugs most users complaint about. See Bug > 362420 which has 64 registered users. I agree it should be possible to hide the Quick Access toolbar item, but at one point we have to stop stacking hacks over hacks. If it's too hard to implement the Quick Access toolbar item correctly (so that it is configurable via the CPD), then we could go with a point fix that just adds a way to remove the badly added Quick Access item.
(In reply to Markus Keller from comment #16) > (In reply to Wojciech Sudol from comment #8) > > 3. After hiding a view toolbar, there is no way to restore it (at least in > > some views). > > This is really a blocker and is enough to justify removal of this feature. Thanks, I missed that issue. I agree that is critical and needs fixing before Luna or this functionality must be removed.h
Bug 431868 and Bug 426535 were intended to allow the user hide the toolbar entries in the windows toolbar because the CPD is broken and the toolcontrols are not yet integrated into the CPD. Especially the ability to hide the Quick Access bar was frequently request. Unintentially this also allowed to hide view toolbars. I don't think any user did ever request the feature to hide a view toolbar. We should prevent that this can happen.
Proposed fix https://git.eclipse.org/r/26234
(In reply to Lars Vogel from comment #19) > Proposed fix > > https://git.eclipse.org/r/26234 -1. This is what I meant in the second paragraph of comment 16. And it doesn't address all the other usability flaws of this menu. An acceptable point fix would just target the Quick Access bar.
(In reply to Markus Keller from comment #20) > -1. This is what I meant in the second paragraph of comment 16. And it > doesn't address all the other usability flaws of this menu. > > An acceptable point fix would just target the Quick Access bar. The patch fixes your critical issue (see comment 17) as requested. In comment 16 you write that this is a hack. I don't think this development is a hack. In the test and development phase several people were involved in it (for example Dani tested it, see Bug 431868) and we did not get this feedback. I would like to ask Paul for this feedback, AFAIK we was also aware of this development.
(In reply to Lars Vogel from comment #21) See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=426535#c10 for the discussion about the approach.
The hacks are that you - stack a second configuration mechanism on top of the Action Set Groups (or whatever it is called the CPD uses) - introduce a UI that doesn't scale Apparently, nobody had enough time to look at these issues upfront. And that isn't a free ticket for anything that comes out of the implementation phase. As I said, an acceptable approach would be to only enable "Hide" for the Quick Access field (rather than disabling it one-by-one for cases where it's known to not work).
(In reply to Markus Keller from comment #23) I would like to ask Paul for his feedback on this development.
I agree with Markus that we can't add a second mechanism that's incompatible with the (admittedly broken) CPD. That it currently has problems doesn't mean we should add a secondary mechanism to do the same thing. That there's not enough time to reconcile them means we should ship without that functionality. Because hiding quick access *is* useful, I'm fine with a point fix that targets the quick access SearchField. We need to remove the original fix (or the parts of the fix that effect other toolbars) and apply it only to quick access. PW
(In reply to Paul Webster from comment #25) > I agree with Markus that we can't add a second mechanism that's incompatible > with the (admittedly broken) CPD. That it currently has problems doesn't > mean we should add a secondary mechanism to do the same thing. That there's > not enough time to reconcile them means we should ship without that > functionality. > > Because hiding quick access *is* useful, I'm fine with a point fix that > targets the quick access SearchField. We need to remove the original fix > (or the parts of the fix that effect other toolbars) and apply it only to > quick access. > > PW Thanks for the clarification. I did not yet look into the option to do a point fix. My current assumption is that this would be definitely an ugly hack, but I might be wrong. Who could do this fix? I'm not sure if my time constraints would allow that for Luna. What would be easily possible it to adjust the development to only allow to hide Toolcontrols in the toolbar. This would include the PerspectiveSwitcher and the QuickAccess but not normal toolbars. We would avoid the overlap as toolcontrols are not yet in the CPD. Let me know if that is an acceptable solution.
(In reply to Lars Vogel from comment #26) > What would be easily possible it to adjust the development to only allow to > hide Toolcontrols in the toolbar. This would include the PerspectiveSwitcher > and the QuickAccess but not normal toolbars. We would avoid the overlap as > toolcontrols are not yet in the CPD. Let me know if that is an acceptable > solution. Yes. Not only "acceptable", but that even sounds like the "right" solution for Luna, since it exactly targets the elements that don't have any configuration mechanism yet.
(In reply to Markus Keller from comment #27) > Yes. Not only "acceptable", but that even sounds like the "right" solution > for Luna, since it exactly targets the elements that don't have any > configuration mechanism yet. Sounds good to me, but IIRC Paul requested that toolbar entries and tool control entries are treated the same, as the user documentation does not make any difference between them. So I would like to wait for Paul's +1 on this. @Paul, is it OK to allow to hide tool controls (not only Quick Access) via this context menu, even if we do not offer that for toolbar entries?
(In reply to Lars Vogel from comment #28) > > @Paul, is it OK to allow to hide tool controls (not only Quick Access) via > this context menu, even if we do not offer that for toolbar entries? I care mostly about Quick Access, but if other Tool controls work the same way and this isn't going to cause problems widening the scope of the fix, I'm OK with that. PW
(In reply to Paul Webster from comment #29) > (In reply to Lars Vogel from comment #28) > I care mostly about Quick Access, but if other Tool controls work the same > way and this isn't going to cause problems widening the scope of the fix, > I'm OK with that. Thanks for the confirmation. I create the fix beginning of next week, hopefully this will still be in time for RC1.
*** Bug 434198 has been marked as a duplicate of this bug. ***
https://git.eclipse.org/r/#/c/26490/ This fix removes the menu from all toolbars but still supports the closing of tool controls via model tags and a context menu. Note: I also allowed to prevent that a tool control can be closed via the IPresentation.NO_CLOSE tag on the related model element. This would be used by RCP applications which don't want to allow that tool controls can be used by the user. I plan to update the CDP dialog for toolbars in 4.5 via to use the IPresentation.HIDDEN_EXPLICITLY tag, instead of its existing logic via Bug 432075. If this works out, I think we should introduce the menu again for toolbars.
(In reply to Lars Vogel from comment #32) > https://git.eclipse.org/r/#/c/26490/ This is better than master, but it still leaves some problems: - When "Preferences > General > Show heap status" is enabled, I can hide the heap status, but then the preference doesn't work any more. (See bug 434840 for issues with "Close" on the heap status item.) => Same problem as for action set groups: Heap status should not be hideable. - When I hide all toolbar entries where the context menu appears (Quick Access, Perspective Switcher, spacer in main toolbar, heap status, progress), then the only ways I found to get the hidden items back is to reset the perspective or open a new window. => The "Restore" command needs to be available in more places than "Hide". I think the best solution is to only enable "Hide" for a fixed set of entries
(In reply to Markus Keller from comment #33) > I think the best solution is to only enable "Hide" for a fixed set of entries I.e. just for Quick Access and maybe the Perspective Switcher. That also solves the problem that the affected entry is unclear: To hide "Quick Access", you have to open the context menu just above or below the text field. If you right-click on the left of "Quick Access", everything looks exactly the same, but "Hide" hides the spacer that keeps stuff right-aligned.
(In reply to Markus Keller from comment #34) > (In reply to Markus Keller from comment #33) > > I think the best solution is to only enable "Hide" for a fixed set of entries > > I.e. just for Quick Access and maybe the Perspective Switcher. Makes sense. I updated the solution to require that a tool control must allow via flag that it can be hidden. This should be changed to a IPresentationEngine constant in 4.5 in as for now I use use a String. https://git.eclipse.org/r/#/c/26490/ For the restore issue, I don't see a good solution in additional to "Reset Perspective". I think we good add a new menu entry but I think that might be overkill for this feature and potentially to late for 4.4.
Pushed patch set 5 for https://git.eclipse.org/r/#/c/26490/ with these changes: - Renamed property to "HIDEABLE" to match IPresentationEngine.HIDDEN_EXPLICITLY and the rest of the implementation in ToolControlRenderer. - Show the "Restore" menu item on all toolbars, but only show "Hide" on hideable entries. This allows the user to hide everything he can, but "Restore" stays available on the spacer (i.e. at the place Quick Access was shown before) and on progress tool bars (not necessary but doesn't hurt). This is good to go for RC1 from my side.
(In reply to Markus Keller from comment #36) > Pushed patch set 5 for https://git.eclipse.org/r/#/c/26490/ with these > changes: > > - Renamed property to "HIDEABLE" to match > IPresentationEngine.HIDDEN_EXPLICITLY and the rest of the implementation in > ToolControlRenderer. > I see no changes in patch set 5 compare to 4. Please check if you uploaded it correctly.
(In reply to Markus Keller from comment #36) > - Show the "Restore" menu item on all toolbars, but only show "Hide" on > hideable entries. I still don't see the changes in Gerrit but I think we should not add the menu to all tool controls by default. Otherwise RCP application will not be able to hide this menu. I suggest to use another tag "ALLOW_MENU" (or something similar) and add it to the spacer control.
Oops, sorry, I apparently just pushed the rebase but not my changes. Patch Set 6 is there now. > I suggest to use another tag "ALLOW_MENU" (or something similar) and add it > to the spacer control. Sounds good. If it's called "ALLOW_MENU", then all "HIDDEN" items need both tags. Better call it "SHOW_RESTORE_MENU" and then use: boolean hideable = toolControl.getTags().contains("HIDEABLE"); //$NON-NLS-1$ boolean showRestoreMenu = toolControl.getTags().contains( "SHOW_RESTORE_MENU"); //$NON-NLS-1$ if (showRestoreMenu || hideable) { createToolControlMenu(toolControl, newCtrl, hideable); } Not posting this as a Gerrit change since I didn't find a good point to add the "SHOW_RESTORE_MENU" tag (doesn't seem to work in the LegacyIDE_fragment.e4xmi when adding it to "Spacer Glue").
(In reply to Markus Keller from comment #39) > Oops, sorry, I apparently just pushed the rebase but not my changes. Patch > Set 6 is there now. > > > I suggest to use another tag "ALLOW_MENU" (or something similar) and add it > > to the spacer control. > > Sounds good. Gerrit review updated. I also checked in the debugger that the tag on "Spacer Glue" is evaluated and the menu is created. Please review.
(In reply to Lars Vogel from comment #40) > Gerrit review updated. I also checked in the debugger that the tag on > "Spacer Glue" is evaluated and the menu is created. Please review. With Patch Set 7, I only see the Hide/Restore menu on Quick Access and Perspective Switcher, but I don't get the Restore-only menu on the spacer. Neither in an old nor in a clean workspace.
I also ran into trouble where I made QuickAccess disappear but was not able to make the perspective switcher disappear or bring back quick access on an existing workbench window (since the tags weren't added to the existing perspective switcher model). Reset Perspective brought the quick access bar back, but of course wiped out my perspective. I find the Restore is really hard to find. I'm willing to entertain 2 options: 1) just quickly fix the perspective switcher tags (make sure they're added). Then at least the menu comes back on the perspective switcher, and opening a new bogus perspective and resetting it will bring back anything as a workaround without losing data. 2) Do #1 and turn the Restore into a command, so it can also be called from quick access. PW
(In reply to Markus Keller from comment #41) > With Patch Set 7, I only see the Hide/Restore menu on Quick Access and > Perspective Switcher, but I don't get the Restore-only menu on the spacer. > Neither in an old nor in a clean workspace. With Patch Set 6 I also don't see the spacer menu. Does that work for you?
(In reply to Paul Webster from comment #42) > 1) just quickly fix the perspective switcher tags (make sure they're added). > Then at least the menu comes back on the perspective switcher, and opening a > new bogus perspective and resetting it will bring back anything as a > workaround without losing data. I don't understand. Tags are already where, see: https://git.eclipse.org/r/#/c/26490/7/bundles/org.eclipse.ui.ide.application/LegacyIDE_fragment.e4xmi,cm > 2) Do #1 and turn the Restore into a command, so it can also be called from > quick access. Sounds good. Can this be done in RC2? I think we close RC1 today and I don't think I will manage to do it today. Maybe the existing change can be merged for RC1 and we do the rest in RC2, I think the current patch is better compared to the existing situation.
(In reply to Lars Vogel from comment #44) > (In reply to Paul Webster from comment #42) > > 1) just quickly fix the perspective switcher tags (make sure they're added). > > Then at least the menu comes back on the perspective switcher, and opening a > > new bogus perspective and resetting it will bring back anything as a > > workaround without losing data. > > I don't understand. Tags are already where, see: > https://git.eclipse.org/r/#/c/26490/7/bundles/org.eclipse.ui.ide.application/ > LegacyIDE_fragment.e4xmi,cm It's https://git.eclipse.org/r/#/c/26490/7/bundles/org.eclipse.ui.workbench/Eclipse+UI/org/eclipse/ui/internal/WorkbenchWindow.java line 846 They're only added in the case of a new perspective switcher. So if you make your quick access disappear in an existing window in an existing workspace, there's no menu on the perspective switcher to bring it back. PW
Created attachment 243100 [details] Restore menu on spacer with patch set 6 (In reply to Lars Vogel from comment #43) > With Patch Set 6 I also don't see the spacer menu. Does that work for you? Yes.
(In reply to Markus Keller from comment #46) > Created attachment 243100 [details] > Restore menu on spacer with patch set 6 > > (In reply to Lars Vogel from comment #43) > > With Patch Set 6 I also don't see the spacer menu. Does that work for you? > > Yes. Sorry, I was clicking on a incorrect position. If I use patch set 7 and click in the empty space I get the "Restore menu". Did you clean you model data with the -clearPersistedState parameter?
Created attachment 243103 [details] Screenshot demonstrating my click
Created attachment 243104 [details] Video in ogv format
(In reply to Lars Vogel from comment #49) > Created attachment 243104 [details] > Video in ogv format This little video demonstrates how I use it. AFAICS all is good with the patch. Please point up what is not working for you.
(In reply to Lars Vogel from comment #50) > (In reply to Lars Vogel from comment #49) > > Created attachment 243104 [details] > > Video in ogv format > > This little video demonstrates how I use it. AFAICS all is good with the > patch. Please point up what is not working for you. Markus, the video is done with patch set 8. Would be great if you can validate that this work now also for you.
Patch 8 doesn't address the problem of opening an existing workspace. The Quick Access fragment gets replaced, so it picks up the new tags. The spacer and the perspective switcher are already there from pre-patch, and so they can't be hidden and/or don't have the restore menu. PW
I've pushed patch set 9 with minor modifications. Lars, can you review it? Then it can go in. PW
(In reply to Paul Webster from comment #53) > I've pushed patch set 9 with minor modifications. Lars, can you review it? > Then it can go in. > > PW Thanks Paul for the help, change looked fine to me. I released the change via https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6ae6324282e2a63a4eb5c70d4e4ca2e5114b553b
(In reply to Lars Vogel from comment #54) With master, I do get the Restore menu on the spacer now, but only in a new workspace. If I launch with an old workspace, I only get a context menu for Quick Access, but not for the Perspective Switcher nor for the spacer. Furthermore, the hidden Quick Access shows up again after a restart. The Perspective Switcher stays hidden across restarts.
(In reply to Markus Keller from comment #55) > (In reply to Lars Vogel from comment #54) > With master, I do get the Restore menu on the spacer now, but only in a new > workspace. If I launch with an old workspace, I only get a context menu for > Quick Access, but not for the Perspective Switcher nor for the spacer. On an existing workspace, they're rendered before we get an opportunity to update the tags. If you restart with the patch once, then they work. > > Furthermore, the hidden Quick Access shows up again after a restart. The > Perspective Switcher stays hidden across restarts. It seems quick access is being removed and re-applied because it's coming in as a fragment. PW
(In reply to Paul Webster from comment #56) > It seems quick access is being removed and re-applied because it's coming in > as a fragment. > > PW I think Tom fragment contribution policy in the org.eclipse.e4.workbench.model extension point should fix that. Do you know how this is supposed to work? I tried to use "initial" and "notexists" but I did not see a difference. I also reduce the priority of the bug to normal, this is not a loss in functionality anymore or does break anything existing.
(In reply to Lars Vogel from comment #57) > (In reply to Paul Webster from comment #56) > > > It seems quick access is being removed and re-applied because it's coming in > > as a fragment. > > > > PW > > I think Tom fragment contribution policy in the > org.eclipse.e4.workbench.model extension point should fix that. Do you know > how this is supposed to work? I tried to use "initial" and "notexists" but I > did not see a difference. I did a little more digging. The fragment contributes TrimContribution. The M*Contributions are remove/re-applied on restart, that's where the problem comes from. On possible workaround would be to save if quick access is hidden on the window, and hide it in org.eclipse.ui.internal.WorkbenchWindow.positionQuickAccess() if necessary. PW
Targetted fix: https://git.eclipse.org/r/26758 If quick access is hidden, store that fact in the window tags as well. On startup when we do quick access pre-processing, re-apply that state if necessary. Markus and Lars, can you review for RC2? PW
(In reply to Paul Webster from comment #59) > Targetted fix: https://git.eclipse.org/r/26758 Behaves as it should on a new workspace, but when I create a new workspace with 4.3.2 and then launch it with master and this patch, then I only get the context menu on Quick Access. Once I hide Quick Access, there's no Restore menu any more.
(In reply to Markus Keller from comment #60) > > Behaves as it should on a new workspace, but when I create a new workspace > with 4.3.2 and then launch it with master and this patch, then I only get > the context menu on Quick Access. Once I hide Quick Access, there's no > Restore menu any more. The restore menu would be present on the next restart. The tags were added in 4.4, but the control had already been instantiated the first time through, so the menu would only appear on the next start. I've updated my fix and added code that correctly adds the menu to the existing toolcontrol when the tags are added and tested it against 4.3.2 -> start once with Luna and it works. PW
+1 for https://git.eclipse.org/r/26758 - 3 The menus look good now and transition from a 4.3.2 workspace also works fine. The only remaining issue I found is for users with multiple workbench windows: If you hide Quick Access in one workbench window, it gets hidden in all workbench windows when you restart.
I've released it as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=488f221b82f7f1298a1a6aea62416785f464ffe7 and I'll double-check with Lars. I've opened Bug 435435 to fix making all quick access controls disappear on restart. PW
*** Bug 436140 has been marked as a duplicate of this bug. ***
