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

enable actionbar buttons without graphview being focused #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bananeweizen
Copy link
Contributor

There are several actionbar buttons which also work fine without the
graphview being focused (e.g. when a code editor currently has focus). Its
those buttons which don't depend on any selection in the graph view.

Therefore always enable those buttons, as that saves the user one click for focusing the graph view:

  • refresh
  • save screenshot
  • toggle link with editor

There are several actionbar buttons which also work fine without the
graphview being focus (e.g. when a code editor currently has focus). Its
those buttons which don't depend on any selection in the graph view.
Therefore always enable those buttons, as they can be used without
focusing the graph view now:
* refresh
* save screenshot
* toggle link with editor
@marcphilipp
Copy link
Member

Well, these are global command handlers but they only work when one of the graph views is active. Thus, I'm not sure we should always enable them.

@Bananeweizen
Copy link
Contributor Author

You are of course right with that complaint.

Right now all the handlers are bound to the action bar buttons of the graph view. The graph view becomes active because of hitting the button (and also before triggering the handler). So a problem would only come up if the handler were actually used with a UI element outside of the graphview, where I believe the handlers just aren't used in such a way. Have I missed something there?

@marcphilipp
Copy link
Member

I think I would rather leave the enablement as is. Ok by you?

@Bananeweizen
Copy link
Contributor Author

Hm, actually I'm not satisfied with that outcome as the graphview behaves really strange and different than every other view, regarding this enablement.
If you feel uneasy about the enablement, then I would rather add code inside the handler to verify that the active part is a graph view. Would that make the change acceptable on your side?

@marcphilipp
Copy link
Member

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants