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

Keyboard shortcuts with Ctrl stopped working #188

Closed
hugoholgersson opened this issue Jan 3, 2023 · 28 comments · Fixed by #199
Closed

Keyboard shortcuts with Ctrl stopped working #188

hugoholgersson opened this issue Jan 3, 2023 · 28 comments · Fixed by #199
Labels

Comments

@hugoholgersson
Copy link
Contributor

hugoholgersson commented Jan 3, 2023

After #181, keyboard shortcuts with Ctrl, also known as ⌃ on macOS, no longer work.

Example:
Before: Control-Up did "Previous Difference"
After: Control-Up moves the cursor one line up.

@MightyCreak
Copy link
Owner

MightyCreak commented Jan 22, 2023

Can you retry please?

I fixed the issue #186 with the PR #189, and I think your bug and #186 are connected.

Nevermind, I think it's another bug linked with the shortcuts and not the toolbar buttons. I tried on Linux and have the same bug

@MightyCreak
Copy link
Owner

Could it be linked to #174 ?

@MightyCreak
Copy link
Owner

@hugoholgersson now that the PR is merged, could you verify that this bug is actually fixed? Thank you 😉

@bongochong
Copy link
Contributor

bongochong commented Apr 3, 2023

I didn't want to create a new issue since you've put a good deal of effort into fixing keyboard shortcuts across different platforms, but the fix for keyboard shortcuts on Mac in 0.8.0 has nullified keyboard shortcuts on Linux (at-least under Fedora 37). I'm making a patch as we speak that will apply to builds in my COPR repo, but it will simply revert this commit, as I'm not sure about other ways to remedy the situation.

P.S. Reverting that commit doesn't seem to restore the functionality.

P.P.S. After further testing, it still seems that keyboard shortcuts for navigating through differences have been completely nuked on Linux (at least on Fedora 37) since commit 8e32f88. This applies to Flatpak releases of Diffuse as well. Builds based on commits prior to 8e32f88 have working keyboard shortcuts, and builds based on that commit or any thereafter do not.

@krlmlr
Copy link
Contributor

krlmlr commented Apr 5, 2023

Is there a difference between Shift+Ctrl+ and Ctrl+Shift+ in that file?

@bongochong
Copy link
Contributor

Is there a difference between Shift+Ctrl+ and Ctrl+Shift+ in that file?

Are you asking me, @MightyCreak, or @hugoholgersson? AFAIC, nothing in 8e32f88 directly references that. @MightyCreak mentioned that some refactoring work might be the cause of keyboard shortcuts and toolbar buttons not being responsive at points since the release of 0.7.7, but I don't know enough about Python or GTK to figure out what happened.

@MightyCreak
Copy link
Owner

Considering the comments, it seems to come from the move to GtkApplication. That said, I think it worked locally... That's weird... I need to do more tests I guess.

One thing for sure, I won't revert this commit as it represents a lot of changes, but also because it should be working... the proper solution is to find what we are doing wrong when setting the accelerators.

I'll look into that this weekend.

@MightyCreak
Copy link
Owner

And thank you very much for the investigation, it really helps!

@bongochong
Copy link
Contributor

And thank you very much for the investigation, it really helps!

Not a problem. I've long preferred Diffuse for a graphical diff application, as it's much lighter and faster than the other popular options for Linux, but still has modern conveniences that make it a good deal more pleasant to work with than programs like tkdiff. I intend to continue maintaining the package for Fedora so long as this project is alive.

Considering the comments, it seems to come from the move to GtkApplication. That said, I think it worked locally... That's weird... I need to do more tests I guess.

One thing for sure, I won't revert this commit as it represents a lot of changes, but also because it should be working... the proper solution is to find what we are doing wrong when setting the accelerators.

I wouldn't expect you to revert those changes, though I'm hoping to devise a set of patches that restores the functionality in question, which I think is necessary before unleashing 0.8.0 into the Fedora repos. If all goes well, this could help you isolate the source of the issue (but I assume you'll figure it out before then). I'll do some further testing under a few different DEs in the mean time too :)

@hugoholgersson hugoholgersson changed the title Keyboard shortcuts with Control stopped working on macOS Keyboard shortcuts with Ctrl stopped working Apr 6, 2023
@hugoholgersson
Copy link
Contributor Author

Thanks @bongochong for sanity testing (and debugging) before shipping to Fedora. Thanks to you, this bug did not slip to Fedora.

@bongochong
Copy link
Contributor

@hugoholgersson It's small potatoes compared to the work done on reanimating Diffuse itself, but thank you and much appreciated. I learned pretty fast to test on at least a couple of devices before doing anything that would make it into the default repos.

@MightyCreak
Copy link
Owner

So I've tested locally and indeed Ctrl+Up etc doesn't work, but n and p do work. So there must be some kind of interference somewhere between:

self.setKeyBinding('menu', 'next_difference', defaultModKey + 'Down')

and:

self.setKeyBinding('line_mode', 'next_difference', 'n')

I'll continue investigating!

@MightyCreak
Copy link
Owner

Unrelated with this issue, but it seems to be the only way right now to contact both of you @hugoholgersson and @bongochong.

I created a Matrix room if you ever want to discuss stuff outside the scope of a single issue: https://matrix.to/#/#diffuse:matrix.org

Also, I migrated the default branch from master to main. For the time being I'm keeping the master branch in sync, but if you have any ref to master in your Fedora script, please change them to main, that should be all to do (as well as switch to the main branch on your clone and delete the master branch).

@MightyCreak
Copy link
Owner

Now more about this issue: I think I found the reason why it doesn't work any more: GTK action names doesn't allow _ in them. I think it's time for the big refactor of the actions names...

@joyously
Copy link

joyously commented Apr 7, 2023

Is the action name the first parameter? Your code reference of what worked and what didn't work show the opposite ( 'menu' didn't work and 'line_mode' did).

@MightyCreak
Copy link
Owner

MightyCreak commented Apr 7, 2023

@joyously the action name is the second parameter, the first parameter is the action context (in Diffuse terms, not GTK). But I found the real reason why!

It was indeed related to the action name, but not related to the menu or line_mode contexts. Only the action names defined in the menu bindings are used with Gio.Action.print_detailed_name which doesn't tolerate _ in the name. The PR above is a quick fix (of a quick fix 😅 ) and this time it properly formats the action name as GTK would expect it.

@bongochong
Copy link
Contributor

All good now. Just uploaded to COPR, and 0.8.1 will make its way into the default repos in a few days. Fantastic. Thank you for the Matrix room and the quick work on this!

@MightyCreak
Copy link
Owner

MightyCreak commented Apr 7, 2023

FYI: I made a better fix (getting rid of the workaround we had), it's merged, but no release for it yet: #202

This PR also fixes an issue with the syntax menu, and probably some other issues as I discovered that some signals haven't been renamed properly when we did the migration to Gtk.Application.

Check the "Unreleased" section in the CHANGELOG: https://github.com/MightyCreak/diffuse/blob/main/CHANGELOG.md#unreleased

@bongochong
Copy link
Contributor

bongochong commented Apr 7, 2023

I'll pump out a build based on that commit into COPR (I'm now realizing that many people don't know what COPR is, so in brief, it's the Red Hat and Fedora equivalent to the PPA system for Ubuntu). If you plan on doing a 0.8.2 release shortly, let me know and I'll hold off on pushing a new stable release to the official Fedora repos until that's rolled out. Many, many thanks for all of these improvements.

P.S. The syntax menu is now working flawlessly.

@MightyCreak
Copy link
Owner

MightyCreak commented Apr 7, 2023

Well I could do a new release very soon, I just wanted first to have some feedback on the latest version on main (because that's a lot of changes in the end). If you could use it a bit and tell me if you see other regressions, that would be greatly appreciated!

PS: I'm using Fedora too, so I know about COPR too (although I don't really use them much as I prefer Flatpak now 😉 )

@bongochong
Copy link
Contributor

Wonderful. I'm already testing it a bit and I'm sure it will see continued use over the next couple of days, so I'll definitely update you if any issues pop up.

@bongochong
Copy link
Contributor

Have run into no issues at all since my last comment. I think that the additional fixes for keybindings and restoration of the syntax choice menu warrant a new release, but it's up to you. I also wanted to point out that Diffuse is now the only graphical diff utility built with GTK that still sports a traditional application menu (as opposed to some counter-intuitive CSD monstrosity), making it the ideal choice for DEs like Cinnamon, MATE, Xfce, Budgie etc...

@MightyCreak
Copy link
Owner

Thanks for the feedback!

For technical reasons, I'll only be able to make a new release next weekend.

About the menu application, I tend to like the GNOME HID. I like that actions must be more contextualized and less exposed all the time at the top. When well-thought, I believe only the relevant actions should be available next to where the user is working. But good UX is very hard to get and needs a lot of iterations.

Traditional application menu, although not a very good UX, has the advantage of being well-known and easy to develop (e.g. you just add a new entry in the already huge menu list, and potentially drop another icon in the menubar, and call it a day).

You can take a look at @mjourdan's proposition of a new UX for Diffuse here: #90. It's quite the change compared to what we have right now, but I do believe there are some very good ideas there.

@bongochong
Copy link
Contributor

Very interesting UX ideas in there, and it would still be a whole lot more accessible and functional than Meld's current design. If it's alright by you, I plan to keep Diffuse at 0.7.7 in the Fedora repos until 0.8.2 is released, simply because if I pushed 0.8.1 now, it would take up to a week before it left the testing phase. If you'd like me to push 0.8.1 anyway, just say the word and I'll roll it out. I would do a git build like I do for COPR (or 0.8.1 + several patches from your latest commits), but the documentation seems to frown upon this for packages in the default repos. Many thanks for all the hard work!

@MightyCreak
Copy link
Owner

I'm perfectly fine with that (the last release was 5 months ago, I think we can wait 1 more week 😄)

@bongochong
Copy link
Contributor

Superb. Next release it is then.

@MightyCreak
Copy link
Owner

@bongochong release 0.8.2 is out!

@bongochong
Copy link
Contributor

@MightyCreak Great! I'll roll out a new git build for the Diffuse COPR repo tonight, then update the spec for the official repos and push new builds there shortly thereafter.

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

Successfully merging a pull request may close this issue.

5 participants