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

Add back missing call to resize #1300

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

jfly
Copy link
Contributor

@jfly jfly commented Feb 11, 2021

The call to resize is pretty important... without it, nothing shows up
on the screen when I run flameshot (presumably because it's 0 pixels by
0 pixels large). This was accidentally removed when resolving merge
conflicts in 77c509e.

While I was in here, I also opted to delete some comments. I personally
am never a fan of commenting out code: if we need something, that's what
Git is for! And, just in case GitHub disappears, I thought it would be
nice to record my research in a Git commit about why I think removing
Qt::BypassWindowManagerHint is ok. Comments copied from
#731):

I played around with it [removing Qt::BypassWindowManagerHint] a bit
and it has some really nice properties for me as a Xmonad X11 user:

I did some git spelunking and here's the history of the current code:

  • c4d9210 is the first commit where
    something like this showed up, but it used
    Qt::X11BypassWindowManagerHint, which is just an alias for
    Qt::BypassWindowManagerHint
  • 0f30529 removed it (yay!)
  • 11b0e2d added in
    Qt::BypassWindowManagerHint with somewhat cryptic message: "Capture
    window showing when mouse events are holded" message. I'm not sure what
    that means.
  • Later, this commit a9b0c21 added a
    #ifdef Q_OS_WIN branch that made it so this
    Qt::BypassWindowManagerHint only happens on Linux, not Windows.

So, since flameshot doesn't currently target OSX, I think this change
only affects Linux. @borgmanJeremy if I did some investigation into how
this behaves with other window managers (and maybe wayland?) would you
be open to merging it up?
#731 (comment)
for more information.

Later, I investigated how things behave on Linux with a non-tiling
window manager:

I just installed xfce and tried this out, and as far as I can tell,
things work great! During a screenshot, I can alt-tab to other windows
and they end up on top of the ongoing screenshot, but I can click back
on the screenshot to continue editing the screenshot (strangely, I
cannot alt-tab back to the window). Keyboard shortcuts work as expected,
and this feels like an improvement in every way, IMO.

The call to `resize` is pretty important... without it, nothing shows up
on the screen when I run flameshot (presumably because it's 0 pixels by
0 pixels large). This was accidentally removed when resolving merge
  conflicts in 77c509e.

While I was in here, I also opted to delete some comments. I personally
am never a fan of commenting out code: if we need something, that's what
Git is for! And, just in case GitHub disappears, I thought it would be
nice to record my research in a Git commit about why I think removing
`Qt::BypassWindowManagerHint` is ok. Comments copied from
flameshot-org#731):

> I played around with it [removing `Qt::BypassWindowManagerHint`] a bit
> and it has some really nice properties for me as a Xmonad X11 user:
>
>   - It resolves flameshot-org#784
>   - It also makes it possible for me to clearly see when flameshot has
>     focus, and give it focus, which I'd argue is a workaround/fix for
> flameshot-org#1072, and also has
> some nice properties as @filipkilibarda mentioned: "Allows the user to
> switch workspaces while in the screenshot GUI..."
>
> I did some git spelunking and here's the history of the current code:
>
> - c4d9210 is the first commit where
>   something like this showed up, but it used
> `Qt::X11BypassWindowManagerHint`, which is just [an alias for
> `Qt::BypassWindowManagerHint`](https://github.com/qt/qtbase/blob/b75d60abd2012d78387ec0751e205aef970a024b/src/corelib/global/qnamespace.h#L247)
> - 0f30529 removed it (yay!)
> - 11b0e2d added in
>   `Qt::BypassWindowManagerHint` with somewhat cryptic message: "Capture
> window showing when mouse events are holded" message. I'm not sure what
> that means.
> - Later, this commit a9b0c21 added a
>   `#ifdef Q_OS_WIN`  branch that made it so this
> `Qt::BypassWindowManagerHint` only happens on Linux, not Windows.
>
> So, since flameshot doesn't currently target OSX, I think this change
> only affects Linux. @borgmanJeremy if I did some investigation into how
> this behaves with other window managers (and maybe wayland?) would you
> be open to merging it up?
> flameshot-org#731 (comment)
> for more information.

Later, I investigated how things behave on Linux with a non-tiling
window manager:

> I just installed xfce and tried this out, and as far as I can tell,
> things work great! During a screenshot, I can alt-tab to other windows
> and they end up on top of the ongoing screenshot, but I can click back
> on the screenshot to continue editing the screenshot (strangely, I
> cannot alt-tab back to the window). Keyboard shortcuts work as expected,
> and this feels like an improvement in every way, IMO.
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.

Window focus gets lost after flameshot exits
2 participants