SecurityCentral / classification-banner

Displays Classification Banner for a Graphical Session
Other
4 stars 7 forks source link

Fix "ESC to hide" functionality #15

Closed bracketttc closed 10 months ago

bracketttc commented 11 months ago

Main goal was to fix the hiding functionality. Other things bothered me along the way.

Changes boil down to:

Tested on Fedora 38.

Closes #12 Closes #14 Closes #16

bracketttc commented 11 months ago

Tested on RHEL7 machine with slower cores and something's causing it to use way too much CPU after restoring from being hidden.

bracketttc commented 11 months ago

Tested on Fedora 38 and RHEL7.

CPU usage is back to idle/background levels because I'm not accidentally making it a busy loop.

bracketttc commented 11 months ago

@redhatrises can you take a look at this?

redhatrises commented 11 months ago

I think I am fine with many of these changes. There are things to do like move to log instead of print, don't allow the user to run a banner over an existing one, seeing if the banner can be on top of the menu/bottom bar.... but that's outside the scope of this PR. One thing I am wondering (without having looked at the gtk docs) is if the text can stay the same opacity while the banner's opacity changes as the text can be hard to read in some cases. I get the feeling that it might not be the case.

redhatrises commented 11 months ago

Also seems as if the banner disappears behind windows instead of being overlaid on top of the windows which is less than ideal (unless configured to do so.)

bracketttc commented 11 months ago

I ran into that, but I thought I had it sorted out. I'll have to take another look.

bracketttc commented 11 months ago

I think I am fine with many of these changes. There are things to do like move to log instead of print, don't allow the user to run a banner over an existing one, seeing if the banner can be on top of the menu/bottom bar.... but that's outside the scope of this PR. One thing I am wondering (without having looked at the gtk docs) is if the text can stay the same opacity while the banner's opacity changes as the text can be hard to read in some cases. I get the feeling that it might not be the case.

At some level, this is a "best-effort" for the general case of the requirement. If you've got better defined criteria, you may be able to do better. For instance, if you're only using gdm as your desktop manager, and gnome-shell as your only available window manager in your deployment, you should almost certainly prefer using the gnome-shell extension as it's more tightly bound with those pieces and therefore better able to work with and around them. Red Hat's official documentation for RHEL8 and RHEL9 actually points readers to that solution.

I ran into that, but I thought I had it sorted out. I'll have to take another look.

I can't repro this issue anymore and adding a hook to track changes to Gdk.WindowState doesn't seem to show the GDK_WINDOW_STATE_ABOVE flag turning off. But my personal machine isn't a representative case because it's Fedora 38. I can poke it during the week on some of work's RHEL7 and RHEL8 machines.

bracketttc commented 11 months ago

Ah, merge conflict. Is this a merge-out or rebase kind of repo?

bracketttc commented 10 months ago

Rebased.