GitSquared/edex-ui

Alt-F4 on other application shuts down edex-ui

sadata7 opened this issue · 19 comments

Technical information

Using version:

  • master (running from GitHub-published source code, currently v2.2.7-pre)
  • latest (latest release, currently v2.2.6)
  • vX.X.X (specify other version)

Running on:

  • Linux
  • Windows
  • macOS

How comfortable you are with your system and/or IT in general:

  • I'm kind of lost, honestly
  • I know what's up, I could help you run some commands or checks
  • My machine is fully under my control, tell me what you need
  • I attended Defcon last year

Problem

Pressing Alt-F4 to shutdown another application (e.g. text editor, calculator, etc.) on the same desktop/workspace as edex-ui also shuts down edex-ui. It doesn't matter if the other app is started from the edex-ui terminal or not.

which desktop manager/distro are you using?

which desktop manager/distro are you using?

Linux Mint 20.1 Cinnamon

Do you happen to use multiple monitors?

No, just a single monitor on this machine. Note that it only happens when the app is on the same desktop.

I think this is a debounce issue (i.e. edex-ui is processing queued up keystrokes) because, once or twice, I was able to hit the Ctrl-F4 key combination very briefly/quickly and it only affected the intended app (e.g. editor, calculator, etc.). But the majority of the time it just passes right through to edex-ui, shutting it down.

Any performance issues with the app or is it running smoothly?

Seems very smooth.

Have you switched the forceFullscreen/allowWindowed settings in edex or are you running it fullscreen, using your WM to move windows on top?

(possibly related code: )

edex-ui/src/_renderer.js

Lines 1104 to 1109 in ec8e383

// Fix #265
window.addEventListener("keyup", e => {
if (e.key === "F4" && e.altKey === true) {
electron.remote.app.quit();
}
});

fullscreen: settings.forceFullscreen || false,

Same behavior whether in windowed or full-screen mode. However, if I open more than one app such that there is another app between (i.e. in the window list) edex-ui and the app intended to receive Alt-F4, then there's no problem (nor does the intermediate app close unexpectedly if I hit Alt-F4 on the other app). So, for example, if I launch edex-ui (from 2.2.6 AppImage file) and then launch gnome-calculator (either from edex-ui terminal or Ulauncher), closing gnome-calculator with Alt-F4 will immediately shutdown edex-ui. However, if I first launch, say, xed editor or gnome-calculator, and then a second instance of either app, then the intermediate app prevents Alt-F4 from being heard by edex-ui. Only the app intended to receive Alt-F4 shuts down and, at this point, focus is received on the intermediate app. However, hitting Alt-F4 on the remaining app will shutdown that app as well as edex-ui. So problem occurs on apps one level higher than edex-ui in the window list. Also, the launch order doesn't seem to matter. If I launch gnome-calculator or xed editor first, and then edex-ui, bringing the other app to the foreground (e.g. via Alt-Tab) and hitting Alt-F4 still shuts down edex-ui.

Looks like fix 265 was specific to Windows. I guess you could update as below so that the fix only applies to Windows. I tested the code below and it resolves the issue on Linux (and Alt-F4 sent directly to edex-ui still shuts it down, as expected). Don't have Windows to test on, but I assume it will work.

// Fix #265
window.addEventListener("keyup", e => {
    if (require("os").platform() === "win32" && e.key === "F4" && e.altKey === true) {
        electron.remote.app.quit();
    }
});

So, for example, if I launch edex-ui (from 2.2.6 AppImage file) and then launch gnome-calculator (either from edex-ui terminal or Ulauncher), closing gnome-calculator with Alt-F4 will immediately shutdown edex-ui.

Very weird behaviour, could you make sure that it doesn't depend on whether you launch new windows from the edex terminal? I'm thinking they may spawn as child processes and cause edex to still listen to keyboard events in the background.

I guess you could update as below so that the fix only applies to Windows. I tested the code below and it resolves the issue on Linux

The fix is only applying on Windows (require("os").platform() === "win32"). Did you mean to say that you removed that check and it worked on your end?

As mentioned, it doesn't matter if the apps are launched from the edex-ui terminal or using Ulauncher or Alt-F2 launcher. Also, as mentioned, they can be launched before edex-ui is even launched.

As for the fix, I added the require("os").platform() === "win32". That's why I said you "could update it as below." The OS check was not part of your original fix (see your post above where you quoted the current code).

Right, sorry for the misunderstanding - and thanks for digging in and finding a fix. I'll update it, unless you want to send a PR.

Right, sorry for the misunderstanding - and thanks for digging in
and finding a fix. I'll update it, unless you want to send a PR.

Not prob. I should have said the "issue" was Windows specific, not the fix. When I looked up #265, it mentioned that it occurred on Windows and I believe you or someone else couldn't reproduce on Linux. I haven't done a PR before, so I can give it a try for this as well as the uptime format later today.

Great! Just remember to make two different PRs. If this is your first time, the GitHub guide is a good place to start.

Great! Just remember to make two different PRs. If this is your
first time, the GitHub guide
is a good place to start.

I created the first PR and submitted. I then recloned from my fork and made the second change, committed and pushed. However, it seems to have automatically pulled in the second change as part of the existing pull request. Not sure how I can separate them. Please advise.

PRs mean "please merge this branch with this branch" (in the one you opened, #1072, the request is "merge my fork's master branch with your repo's master branch"). So if you add commits to the branch they will be added to the changes proposed in the PR.

The solution is to make different branches on your end to separate the new features/fix, for example fix-265 and feat-better-uptime. Then you can send two different PRs and I can review them independently.

Should be in good order now. Thanks.

@sadata7 Thanks for taking the time to do this. I've merged both PRs, you're now officially a contributor! 🎉