Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected onDidChange Behavior in Sidepanel Toolbars #8970

Closed
colin-grant-work opened this issue Jan 19, 2021 · 1 comment · Fixed by #9798
Closed

Unexpected onDidChange Behavior in Sidepanel Toolbars #8970

colin-grant-work opened this issue Jan 19, 2021 · 1 comment · Fixed by #9798
Labels
bug bugs found in the application toolbar issues related to the toolbar

Comments

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Jan 19, 2021

Bug Description:

The side panel toolbars do not update as expect on onDidChange events associated with the toolbar items currently displayed when the widget occupying the side panel is a ViewContainer.

Steps to Reproduce:

  1. Add a stateful flag to a widget that is displayed as part of a ViewContainer (e.g. the navigator)
  2. Create two commands that manipulate that state and then fire an event.
  3. Make the visibility/enabled functions of the commands depend on the flag on the widget so that exactly one command should always be visible.
  4. Create two tabbar items, one for each of the commands and add an onDidChange for the event fired in step 2.
  5. Open the widget on whose state the commands depend. You should see one tabbar item displayed in the top bar.
  6. Activate the command.
  7. Observe that the visible command disappears, but the other command does not immediately appear.
  8. Focus the widget.
  9. Observe that the second command appears after focus.

Additional Information

bad-side-tabbar

What's happening is this:

  1. Under certain conditions, a ViewContainer runs updateTitle
  2. During that function, if it detects that it has only one visible child, it determines what toolbar items would be visible for that child widget at that moment and registers new commands and toolbar items associated with the ViewContainer itself.
  3. When a didChange event is fired by the TabbarToolbarRegistry, that event is not handled by the ViewContainer but by the SidePanelToolbar
  4. The SidePanelToolbar doesn't know about the gymnastics that the ViewContainer has performed, and since the ViewContainer didn't register a handler for the command that wasn't visible at the moment it set the toolbar up, the side panel can't find such a command, and the second command doesn't appear.
  5. Focusing the visible part of the ViewContainer triggers another run of ViewContainer.updateTitle, and the command that should be visible finally gets registered and does appear.

What should probably happen is that if the ViewContainer detects that it has only one visible child, it delegates its toolbar duties completely to the child, rather than simulating delegation by retrieving items from the TabbarToolbarRegistry on the widget's behalf.

An associated problem is that the SCM widget works around this by extracting the widget from the ViewContainer. This is undesirable because it means that even if another widget is added to the ViewContainer, the icons associated with the SCMTreeWidget will always appear in the top sidebar, rather than in the toolbar associated with that widget as part of the view container.

  • Operating System: RHEL
  • Theia Version: master (1.9.0)
@colin-grant-work colin-grant-work added bug bugs found in the application toolbar issues related to the toolbar labels Jan 19, 2021
@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Jan 20, 2021

In the end, the most straightforward solution (is just to handle TabbarToolbarRegistry.onDidChange in the ViewContainer itself. It might still be desirable to delegate rather than register new commands, but that would require significant rethinking of the current code. I've added the listener to the code in #8966.

Adding that listener causes circular events, so another solution is called for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application toolbar issues related to the toolbar
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant