Feedback on tweaks and features

[stdout-se]

Hey,

I made some changes for my own purposes at work, and wanted to check if you're interested that I continue working on them to perhaps upstream them? Looking for feedback about improvements, and which ones you're interested in (if any) 😄

I've made a few branches that are based one ontop of the other for now, so this branch shows all the commits in one.
https://github.com/stdout-se/gitfourchette/commits/wip/diff-hotkey

Let me know your thoughts. Best regards!

Patch A: Feature: Add support for Commit Signoff

• Adds a checkbox to the commit form that runs git commit --signoff on the commit. This is a requirement for some places to allow pushing to origin etc. This checkbox has to be ticked every time you want to signoff a commit, it's never checked by default.
• Also adds an option in the settings to enable/disable the feature (disabled by default)
• Read more in the git documentation
• 

Patch B: Feature: Find commits that contain changes to a path

• Adds a new search bar that allows users to search for paths, e.g. "*/application.py". All commits that contain changes to that path are highlighted in the commit tree.
• Also allows filtering, so only matching commits are shown in the tree.
• There are some WIP notes in the commit message.
• 
• 

Patch C: Fix unit tests for Cinnamon desktop

• Unit tests were broken for various reasons. Linux desktop environments tend to report custom XDG_CURRENT_DESKTOP. In my case (Linux Mint) it runs Cinnamon, which reports "X-Cinnamon", but there are many others. Took a stab at finding a more generic one.
• Not sure this is interesting to upstream because tests may not have to work on all possible systems. I just fixed what didn't work on my machine.

Patch D: Add support for freedesktop terminal "Terminator"

• It does what it says in the title 😄

Patch E: Add hotkey F4 to open external difftool

• I find myself opening the external difftool quite a lot, so I prefer to have a hotkey for it.
• If a file is selected in any of the file lists, or if the diff for a file is selected, pressing F4 opens the configured external difftool.

[jorio]

Thank you for these patches and your attention to solid tests! I have included C and D into v1.7.0 and I'll review the rest later, but I can already say that A and B look very useful.

Some notes about C: Good catch, I included your FindNext/FindPrevious fix, and I changed the window activation fixes to be a bit less intrusive (8bad8bb) - simply waiting on the window to become active without forcing it manually seems to work, at least in my environment. Please let me know if that wasn't enough in yours.

Not sure this is interesting to upstream because tests may not have to work on all possible systems.

My guideline is that the test suite should pass in offscreen mode because it provides a reliable platform-independent base. I don't expect tests to be 100% reliable in non-offscreen mode due to DE-specific flakiness, though patches like yours to fix one-off issues are welcome. (An example of non-offscreen flakiness that we can't do much about: some tests need to assume complete control of the mouse cursor, which is fairly reliable on X11, but not on Wayland if the mouse cursor happens to be outside the window before the test starts.)

[jorio]

By the way, until E gets merged, you may find it useful to open the diff tool by double-clicking in the file lists. To do so, go to Settings -> Mouse Shortcuts and rebind the double-click action (possible since v1.7.0)

[jorio]

An update on patch B (file search): I think it's a useful feature and I can tell that you put a lot of care into it.

From a UI perspective, I think it'd be more intuitive if there was just a single search bar attached to GraphView. This spurred me to rework the existing SearchBar so that it's easier to write modular "search providers" for it. GraphView's SearchBar could then handle both "find by message" and "find by pathspec". By default, find by message is active, but the user can type a prefix to kick off a file search, like /f *.py (I'm not married to this specific prefix yet).

I've cherrypicked your patch and reworked it with this approach in mind. I think it's working pretty well, I just need to polish it up a bit and I'll share my work in progress soon. Thanks!

[stdout-se]

Hey, a bit busy with life but I'm gonna try to follow up on these soon. Thanks for the feedback and work!

[stdout-se]

I like the idea of having prefixes. Reminds me a litte of how VSCode's Command Palette works. Could be a similar situation with a quick-key to open the palette. I think the issue with having prefix for e.g. finding files is not being able to search for the prefix string in commit messages. I guess that's why VSCode defaults to file search when there's no prefix, and string is prefixed with %

[stdout-se]

@jorio Is there anything to discuss about patch E here? Getting some time to work on this again. Have other ideas I'll upload soon too :)

[jorio]

Also, I still think signoff would be useful to have (I just haven't gotten around to focusing on it yet :))
I had some thoughts about UI/presentation which I'll share soon.

[stdout-se]

Since View-Blame is Ctrl-L, I think there's a little feeling of two competing patterns here? I see that the hotkey combinations are a little busy, so it's tricky. Also Ctrl-D can close terminals if you have a terminal window in focus by mistake. Don't know if that's something to care about.

Adding External Diff to the View menu sounds like a good idea. Is it a good idea to grey out options that are unavailable? I.e. Blame and External Diff requires a file to be selected. If looking at a clean working directory, the menu option is disabled. Right now, Blame just raises an exception AttributeError: 'NoneType' object has no attribute 'old'.

[stdout-se]

Testing out your path search implementation now for a couple of days. So far it feels good. I miss the little truncated color lines for branches when i filter path matches, but that's minor :)

[jorio]

So far it feels good.

Cool!

I miss the little truncated color lines for branches when i filter path matches

I do, too, but the graph can become misleading when random rows are hidden. Consider this:

a ┯
b │ ┯
c ┿─╯
d │ ┯
e │ ┿
f ┿─╯

If we hide 'c' and 'd', the graph would look like this:

a ┯
b │ ┯
e │ ┿
f ┿─╯

It now looks like 'e' is an ancestor of 'b', but that's incorrect. I think we shouldn't show info that could be misconstrued, so I elected to hide the graph when filtering for pathspecs.

Right now, Blame just raises an exception AttributeError: 'NoneType' object has no attribute 'old'.

Good catch, that case slipped through my radar when I implemented the blame feature, and it should definitely be fixed and tested. Hopefully that's the only misbehaving menu item 👼

Since View-Blame is Ctrl-L, I think there's a little feeling of two competing patterns here?

The pattern I was picturing is, when an action can be done both internally or with an external tool, then Ctrl+letter invokes the internal action, and Ctrl+Modifier+letter launches the external tool. In that sense:

• Ctrl+O - Open repo internally
• Ctrl+Shift+O - Open repo in external file manager
• Ctrl+Alt+O - Open repo in external terminal
• Ctrl+D - Detach diff window internally (tentative)
• Ctrl+Shift+D - Diff in external diff tool (tentative)

In any case, this might call for application-wide rebindable keyboard shortcuts later on ;)

Is it a good idea to grey out options that are unavailable?

In the main menu, I feel like that would take more work than it's worth. Nearly all items in Edit, View, Repo, and Commands would need to be reworked if we're going to apply this consistently. As long as the app responds gracefully when the action is unavailable, I'm fine with leaving the global menu bar as it is. The menu bar probably doesn't get a lot of use once a user has found their footing - I mostly see it as a collection of easily discoverable keyboard shortcuts.

Context menus are more frequently used, and they're generated on-demand, so it's easier and more interesting to disable options there. A downside to graying out "external diff" in FileList's context menu is that the user won't be able to get feedback as to why something can't be diffed unless they use the keyboard shortcut.

I don't feel strongly either way. I think the current approach (never gray) is fine, but you can go for it if you feel it'd be an improvement to gray out "external diff" if it's unavailable. We might need a couple tests to check that it's correctly disabled, though.

[stdout-se]

It now looks like 'e' is an ancestor of 'b', but that's incorrect. I think we shouldn't show info that could be misconstrued, so I elected to hide the graph when filtering for pathspecs.

Yeah. Makes sense. It should be easy to use without explaining too much. 😄

In any case, this might call for application-wide rebindable keyboard shortcuts later on ;)

I think a keybinding settings tab would be a nice feature! +1 vote from me hah.

In the main menu, I feel like that would take more work than it's worth.

Yeah I think it would probably touch filelist, mainwindow and repowidget. Feels like more work than it's worth at this stage. Maybe an error message "No file is selected" or similar would be better.... And fewer lines 😄

[stdout-se]

Added some new pull requests, I figured it's easier than to discuss everything in the same thread :)

[jorio]

@stdout-se Re: Signoff: I pushed my current take on the UI to wip/signoff.
I moved the button next to the GPG button, that way it creates a "signing options" cluster of sorts.