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

[UI Feature 3] Shortcut to switch tabs #2755

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mohamedAdhamc
Copy link
Contributor

@mohamedAdhamc mohamedAdhamc commented Jul 19, 2022

Feature Description: Ctr+t to switch tabs of the dockbook which has focus. Of course the dockbook it self doesn't directly have focus (unless the tab has focus I think if i remember correctly). so basically the dockbook that has focus is the dockbook that has an element that has focus. For example the drawing area in the main window, the treeview in the parameter panel, anything really.

Now this was supposed to be Ctr+tab but setting the accelerator to be that gives an error
warning: Invalid accelerator: <Primary><tab> (for action: <Actions>/tab_actions/switch-tab)
and also I think Ctr+tab might have been a problem if it actually were it as Ctr+tab is already used for switching focus.

Anyways, best thing about the preference dialog is that the user can set it to whichever they feel their comfortable with so the initial setting here I think is no huge deal.

p.s. the issue @ice0 noted with the first tab of the animation window not being show in the canvas panel (cant remember what its name was (^_^) ) is of course also still here as it was fixed in feature 3. so feature three needs to be merged before this one. Also just realized there is a bool i forgot to move line 68 switch_page I will remove it before I finish today if i dont forget.

@mohamedAdhamc mohamedAdhamc requested a review from ice0 July 19, 2022 00:56
@mohamedAdhamc mohamedAdhamc changed the title Shortcut to switch tabs [UI Feature 3] Shortcut to switch tabs Jul 19, 2022
@rodolforg
Copy link
Contributor

Hello, @mohamedAdhamc !
There is a better way to do it: App singleton (app.h) has some useful methods:

  • App::get_selected_instance()
  • App::set_selected_instance()

It also has a instance_list ;), so you can know who is next/previous

@mohamedAdhamc
Copy link
Contributor Author

mohamedAdhamc commented Jul 19, 2022

Hello @rodolforg ! hope you're doing well

(app.h) has some useful methods:

  • App::get_selected_instance()
  • App::set_selected_instance()

It also has a instance_list ;), so you can know who is next/previous

sounds good :) I'm assuming this would be for getting the dockbook instances instead of what I did with pointing to them and storing in a vector correct ?.

Anyways I will look into these methods to see how to use them and switch over, maybe at the end of today or maybe tomorrow, for now I'm looking into the history issue.

@rodolforg
Copy link
Contributor

Hmm. You wanted to do in any dockbook? Sorry I missed that.

@mohamedAdhamc
Copy link
Contributor Author

mohamedAdhamc commented Jul 19, 2022

Hmm. You wanted to do in any dockbook? Sorry I missed that.

no worries :), yes it was suggested by @ice0 for this to be for all dockbooks.

@rodolforg
Copy link
Contributor

The focused dockbook wouldn't be the one where shortcut is called?

@mohamedAdhamc
Copy link
Contributor Author

mohamedAdhamc commented Jul 19, 2022

The focused dockbook wouldn't be the one where shortcut is called?

the way I understood it is that the dockbook which has focus in one of its elements is the one that the shortcut should be applied to. Kind of like if there are multiple browser windows the one which has focus would be the one he shortcut is applied. Do you mean something else by the dockbook which has focus?

p.s.
this behaviour i think is similar to how it behaved when i first did this by simply overriding the dockbooks on_key_pressed_event()

@rodolforg
Copy link
Contributor

I think you over-complicated stuff here :)

@mohamedAdhamc
Copy link
Contributor Author

I think you over-complicated stuff here :)

oops :')... so how do you think it should behave ?

@ice0
Copy link
Collaborator

ice0 commented Jul 27, 2022

I noticed in this discussion (https://gitlab.gnome.org/GNOME/gtk/-/issues/4518) that libhandy somehow handles Ctrl+Tab for tabs. Can you check how they did it?

https://gitlab.gnome.org/GNOME/libhandy

@mohamedAdhamc
Copy link
Contributor Author

mohamedAdhamc commented Jul 28, 2022

I noticed in this discussion (https://gitlab.gnome.org/GNOME/gtk/-/issues/4518) that libhandy somehow handles Ctrl+Tab for tabs. Can you check how they did it?

@ice0
ok, I actually spent lots of time looking as how libhandy and other GTK/GNOME apps deal with this limitation.

1- regarding how libhandy does this, they connect the "notebook type widget -not a GtkNotebook- " key pressed signal to a call back function to handle the different types of key events. the intresting part is when the event is of state- control and keyval- tab this happens state = GDK_CONTROL_MASK | GDK_SHIFT_MASK; which although I dont fully understand why especially this but I believe -and correct me if im wrong- but this seems to be changing the state of the event into a totally different control mask so that it is no longer ctr+tab in the event and the focus control switch doesn't happen.

I tried doing something similar to test this so while overriding the dockbooks on_key_pressed() method I caught when the event is ctr+tab then changed the events state similarly. an intresting observation is that now when the tabs have focus ctr+tab works to switch them and not switch focus to next control as it did before doing that.

As to how we might implement something similar in Synfig:
-the shortcut dialog refuses to accept ctr+tab or tab in general as an accelerator as the tab is reserved for gtk, we can work around that in a rather "dirty/patchy" way by not registering this ctr+tab as an accelerator but only to show it in the dialog and then set a flag to activate our block of code whether in the dockbooks key pressed method or elsewhere. But then this would almost entirely disable the use of ctr+enter for focus control for which maybe we can notify the user that by doing so that is the consequence.

2- other than libhandy I have found many other "GTK apps" have had similar problems most of them seem to be using ctr+up/down for switching tabs. one of them ended up allowing it through a PR feat:Add support for tabs in keyboard shortcuts. others have complied with the GTK policy for tab and not tried to get around it, although the users suggested using something like < Primary >grave which ends up being in most keyboards ctr+tab+key above it, which is close enough and at the same time no GTK behavior is being broken.

so I guess now its up to you guys to decide what is more important here, using ctr+tab for switching tabs and partially sacrificing the standardized ctr+tab for focus switching or just going for a close alternatve such as ctr+grave.

my personal opinion: have this feature go through with the close alternative < Primary >grave as the default accelerator so that the functionality for being able to set a keyboard shortcut for switching tabs is present, and then if it is decided that we should get around the gtk tab accelrator issue a sepearate PR could try to allow the general use of tab as an accelerator to be used in the prefrences dialog somehow.

@ice0
Copy link
Collaborator

ice0 commented Jul 28, 2022

Wow! Thanks for investigating.

I suggest then stop for now until a decision is made.

@mohamedAdhamc
Copy link
Contributor Author

Wow! Thanks for investigating.

I suggest then stop for now until a decision is made.

no problem 😄, ok sounds good !

@mohamedAdhamc
Copy link
Contributor Author

@ice0 should we just have this done for Ctr+t/ any other shortcut which can also be changed from the preference dialog. so we have a switch accelerator in place to match #2731 for switching pages?

Then possibly if its decided, we must use ctr+tab despite the possible risks of going against GTK behaviour/design then the this can just be reconfigured .

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.

None yet

3 participants