Skip to content

Add open in external editor QMenu item#118

Merged
jorio merged 9 commits into
jorio:masterfrom
Genfood:open-in-external-editor
Jul 16, 2026
Merged

Add open in external editor QMenu item#118
jorio merged 9 commits into
jorio:masterfrom
Genfood:open-in-external-editor

Conversation

@Genfood

@Genfood Genfood commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Description

This PR adds a new "Open in external editor" option to the Repo menu (QMenu).

🛠️ Changes Made

  • Added the "Open in external editor" item under the Repo menu.
  • Integrated a new SVG icon sourced from Font Awesome (Code Icon).

❓ Questions & Considerations

  • Icon Licensing & Style: Please let me know if this Font Awesome icon aligns with the project's styling and licensing requirements.
  • Contributor Guidelines / README: Should we add a section to the README.md (or a CONTRIBUTING.md) outlining which icon libraries contributors should stick to? This would help keep the UI consistent and clarify licensing for future changes.

💡 Future Idea / RFC (Request for Comments)

I originally considered making the menu icon dynamic based on the user's selected editor (e.g., displaying a VS Code icon for VS Code, a Neovim icon for NVIM, etc.).

However, the current ActionDef class doesn't support this dynamic behavior out of the box. I opted not to alter the architecture of ActionDef without your guidance. Let me know if you would like to see this implemented, and I can open a separate follow-up PR for it!

📷 Visuals

The screenshot below demonstrates the new QMenu item with VS Code configured as the selected external editor:

image

Add a QMenu Item, that will open the current repo inside the configured external edito.
Add a new icon, for the QMenu Item.

@jorio jorio left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! A few notes:

  1. Since the menu item contains the name of the editor, it would be great to sync the label when the user changes their editor setting. See PrefEffects.RebuildMenu.

  2. If the user hasn't configured an editor (settings.prefs.externalEditor is empty), GitFourchette starts the system's default application associated with the path that's being opened. When opening a directory, the system default app is a file manager, not an editor! This is confusing, so I suggest stopping with an error in this case. You can use setUpToolCommand(..., "externalEditor") for a ready-made dialog that prompts the user to set up their editor.

  3. Please add a test so we don't break this feature in the future. You can flesh out testTabBarActions, where similar actions are already being tested. You can set the editor to editor-shim.py which spits the arguments back to a scratch file. (Open Terminal is already tested like this.)

  4. I make all icons by hand for consistency, and to skirt licensing issues. I'll push one when I have time. Maybe prefs-diff is a good enough placeholder in the meantime.

  5. Editor-specific icons would be nice, but yes, let's think about those later. Maybe we can use something like QIcon.fromTheme("kate"), which also works with Flatpak IDs. We could factor the "interesting token extraction" out of ToolPresets.getCommandName to get the correct token to pass to QIcon.fromTheme.

Comment thread gitfourchette/mainwindow.py Outdated
Comment thread gitfourchette/mainwindow.py Outdated
@Genfood
Genfood marked this pull request as draft July 13, 2026 12:54
@Genfood
Genfood marked this pull request as ready for review July 13, 2026 12:56
@Genfood

Genfood commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

I guess I changed all of your remarks.
Edit: I don't know why the GPG test timed out, but I don't think that's related to my test changes? 😄

@jorio

jorio commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Thanks! Can we also ensure that the workdir was actually passed to the editor? Something like assert Path(wd).samefile(editorShimResult[1]) maybe?

I'd also like to cover the code path when the user has no editor set. You can clear the editor pref and do rejectQMessageBox(mainWindow, r"text editor.+isn.t configured") to ensure that the dialog did appear. After that I think we're good for merging.

I don't know why the GPG test timed out, but I don't think that's related to my test changes?

Unrelated, don't worry! I guess the timeout values aren't generous enough for when the CI runner happens to be under heavier load than normal.

@Genfood

Genfood commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

I noticed two things while working on the tests:

1. VS Code Formatting and Ruff

Ruff seems to be configured primarily for linting. But because I have format-on-save enabled globally, it caused some formatting conflicts on my end. To prevent this, we could add the following to .vscode/settings.json to disable format-on-save for Python:

"[python]": {
  "editor.formatOnSave": false
}

Alternatively, we could enforce a specific formatting style so that running Ruff does not conflict with existing code?

2. Offscreen Test Execution on Wayland

When running tests inside VS Code on Wayland, the QT_QPA_PLATFORM environment variable is not set to offscreen. This led to issues grabbing certain UI elements.

Is it intentional that tests do not run offscreen when executed without debugging? I noticed QT_QPA_PLATFORM is set to offscreen in several places (such as pytest.run.xml), but VS Code does not read these. There also seems to be some configuration in test.py.

To resolve this, I propose unifying this configuration at the top of conftest.py:

# --- Force Qt to run offscreen for all automated tests ---
os.environ["QT_QPA_PLATFORM"] = "offscreen"
# ----------------------------------------------------------

- Add test for editor not set
- fixes test to ensure also the wd
@Genfood
Genfood requested a review from jorio July 15, 2026 12:42
@jorio
jorio merged commit 8998236 into jorio:master Jul 16, 2026
7 checks passed
@jorio

jorio commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Thank you for the PR and your notes!

we could add the following to .vscode/settings.json to disable format-on-save for Python

Sure! Feel free to submit a PR to improve the VS Code config. I supplied a basic set of config files for VS Code, but it's not my primary editor so there's probably room for improvement.

we could enforce a specific formatting style so that running Ruff does not conflict with existing code?

Yes, ruff is only used for linting for now. Long term, enforcing a formatting style might be good to do. The current style isn't 100% consistent so I'll probably still have to format the entire codebase at some point. I'll think about it.

QT_QPA_PLATFORM ... is not set to offscreen. This led to issues grabbing certain UI elements.

Yeah, Wayland introduces a few pitfalls when tests are run visually, due to their restrictions on mouse cursor control (compared to X11 or macOS). That's why the offscreen mode is the only "source of truth", and visual tests are not reliable.

(For what it's worth, the workaround on Wayland is to position the mouse cursor on top of the test window, not over another application.)

Is it intentional that tests do not run offscreen when executed without debugging?

When I flesh out a new test, I like to run it visually so I can see the state of the UI. In doing so, I sometimes use pauseDialog() to explore the UI without locking up the application.

When it's time to check that everything actually works, I use test.py to run the tests offscreen (more options available, see --help). The CI also runs test.py in pull requests.

pytest.run.xml is a PyCharm configuration to run the tests offscreen inside the IDE. Perhaps you can create an offscreen config for VS Code?

To resolve this, I propose unifying this configuration at the top of conftest.py:

I don't think we should override the developer's env variables. This would break the workflow I described above ;)

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