Flameshot: The edit buttons show up inside the screenshot area, making it hard to edit the content.

[jeet-parekh]

Describe the bug
Sometimes after taking a screenshot, the edit buttons show up inside the screenshot area, which makes it hard to edit the content behind the buttons. This screenshot was taken on the 2560x1440 monitor.

To Reproduce
It happens randomly. Sometimes immediately after taking the screenshot. Sometimes after taking a screenshot and resizing the selected area.

Expected behavior
The buttons should be outside the screenshot area.

System Information
Fedora 32
Gnome 3.36.6

$ flameshot --version
Flameshot v0.8.3
Compiled with Qt 5.14.2

$ loginctl show-session $(loginctl show-user $(whoami) -p Display --value) -p Type --value
x11

$ xrandr --listactivemonitors
Monitors: 2
 0: +*HDMI-1 2560/597x1440/336+1366+0  HDMI-1
 1: +eDP-1 1366/309x768/173+0+672  eDP-1

$ xrandr | grep -v " disconnected "
eDP-1 connected 1366x768+0+672 (normal left inverted right x axis y axis) 309mm x 173mm
   1366x768      60.06*+
   1280x720      60.00    59.99    59.86    59.74  
   1024x768      60.04    60.00  
   960x720       60.00  
   928x696       60.05  
   896x672       60.01  
   1024x576      59.95    59.96    59.90    59.82  
   960x600       59.93    60.00  
   960x540       59.96    59.99    59.63    59.82  
   800x600       60.00    60.32    56.25  
   840x525       60.01    59.88  
   864x486       59.92    59.57  
   700x525       59.98  
   800x450       59.95    59.82  
   640x512       60.02  
   700x450       59.96    59.88  
   640x480       60.00    59.94  
   720x405       59.51    58.99  
   684x384       59.88    59.85  
   640x400       59.88    59.98  
   640x360       59.86    59.83    59.84    59.32  
   512x384       60.00  
   512x288       60.00    59.92  
   480x270       59.63    59.82  
   400x300       60.32    56.34  
   432x243       59.92    59.57  
   320x240       60.05  
   360x202       59.51    59.13  
   320x180       59.84    59.32  
HDMI-1 connected primary 2560x1440+1366+0 (normal left inverted right x axis y axis) 597mm x 336mm
   2560x1440     59.95*+
   1920x1080     60.00    50.00    59.94  
   1920x1080i    60.00    50.00    59.94  
   1680x1050     59.88  
   1600x900      60.00  
   1280x1024     75.02    60.02  
   1280x800      59.91  
   1152x864      75.00  
   1280x720      60.00    50.00    59.94  
   1024x768      75.03    60.00  
   832x624       74.55  
   800x600       75.00    60.32  
   720x576       50.00  
   720x576i      50.00  
   720x480       60.00    59.94  
   720x480i      60.00    59.94  
   640x480       75.00    60.00    59.94  
   720x400       70.08  

[mmahmoudian]

@jeet-parekh does it happen on both monitors?

[jeet-parekh]

@mmahmoudian, it hasn't happened on the smaller screen so far. But it happens often on the bigger one.

[mmahmoudian]

@jeet-parekh would it still happens if you reduce your monitor resolution (for example to 1280x1024)?

[jeet-parekh]

@mmahmoudian, yes. This is on 1280x1024.

[borgmanJeremy]

Is fractional scaling enabled?

[jeet-parekh]

@borgmanJeremy, nope.

[borgmanJeremy]

Interesting, I am trying it in a Fedora32 VM with the same resolution and can't make it happen. Maybe I can add some debug print statements to get a more detailed log from your pc.

[jeet-parekh]

Not sure if it's because of the resolution, or multiple monitors.
Sure, I'll run flameshot with detailed logging. Point me to the branch/commit when it's ready.

[borgmanJeremy]

You win the bug of the year! I started instrumenting the software to print out debug information and I found the root cause (no fix yet).

The button algorithm works by checking if there is enough blank space outside the selection to draw the buttons. Once we run out of outside space it draws it to the inside.

The key thing that seems to be wrong is we grab the resolution from screen 0 when making the calculation. I suspect your smaller monitor is is screen 0.

Then if you start drawing on screen 1 that has a higher resolution, you are outside the bounds from screen 0. Not 100% sure of the fix yet but I'll get this added to the backlog.

[jeet-parekh]

@borgmanJeremy, great find!

I suspect your smaller monitor is is screen 0.

You're right. Screen 0 is the laptop screen which is always 1366x768. And the other one is an external monitor which is always 2560x1440.

The button algorithm works by checking if there is enough blank space outside the selection to draw the buttons. Once we run out of outside space it draws it to the inside.

If you look at selected areas in my screenshots above, https://github.com/flameshot-org/flameshot/issues/1070#issue-722058950 is of size 1038X336, and https://github.com/flameshot-org/flameshot/issues/1070#issuecomment-709021112 is of size 944x202. Both are smaller than my laptop screen (1366x768). Is it safe to say that the second selection area is significantly smaller...? Considering that, do you think the algorithm should actually be detecting "enough" blank space outside the selection? Just a thought. You'd know better.

Another interesting observation. While trying to reproduce the issue on a smaller resolution for https://github.com/flameshot-org/flameshot/issues/1070#issuecomment-709021112, I discovered that after reproducing this issue on the external monitor (with buttons inside the selection area)... if I drag the selection area to the laptop screen, the buttons move to outside of the selection area.

[borgmanJeremy]

@jeet-parekh Good thoughts, the algorithm should 100% detect enough free space. I finally have a good setup for troubleshooting this. When debugging this application you really need to run it in a VM so breakpoints don't freeze the system :)

I had some difficulties getting multimonitors set up in QEMU which is key to triggering this issue but should be good to go now.

[jeet-parekh]

@borgmanJeremy, I feel like it's a bit easier to reproduce if you take a screenshot from the common edge in the second monitor. This selected area is definitely larger than my screen 0. But, if I drag the selected area to the middle of the second screen, then the buttons are drawn outside the selected space, which is correct.

[borgmanJeremy]

Analysis

When multimonitors are used in flameshot, break the screen into QRegions
https://github.com/flameshot-org/flameshot/blob/2d4cd76e85ba2dc4f03482dce21de5ca436d8c3b/src/widgets/capture/buttonhandler.cpp#L390-L396

The += operator for a QRegion returns a union of the two regions. However this is returned as a vector of rectangular regions (will be important).

For a simple example see below:

You can imagine this gets much more complicated the more monitors you have and the less the are aligned. For instance, in the configuration below is the same resolution, but different monitor positions return a different number of boxes.

Here is where the bug manifests itself. When a user selections a region for a screenshot, the program looks at the intersection of the selected region against the "screen regions" shown above. Which ever screen region has the highest intersection is chosen for button drawing:

https://github.com/flameshot-org/flameshot/blob/2d4cd76e85ba2dc4f03482dce21de5ca436d8c3b/src/widgets/capture/buttonhandler.cpp#L240-L248

In the diagram below, imagine the screenshot is the blue region. This has the most overlap with the red screen region, so this is chosen. However, the screenshot extends past the bottom. Therefore when the buttons are drawn, the positioning algorithm thinks we are against the bottom of a screen so there must not be room to draw on the bottom, therefore it puts the buttons above the bottom. If you were truly against the bottom of the monitor this is what you want.

I believe the fix will be to replace the += operator() used in the screen regions with .push_back(). Then the screen regions will be literally the position and size of the monitors, NOT the logical intersection. I will make this change soon and get it tested.

[mmahmoudian]

Then I have one question, how will the software act when the blue rectangle is partially on one monitor and the rest is on one or more other monitors when you change to .push_back()?

[borgmanJeremy]

Ooof, thats a good point, It will basically be a bug in a different way since it will treat the monitor edge as a border. This may already be a requirement on Wayland and Windows though. I'll so some research on if I can get a non-rectangular geometry working on wayland and windows.

[jeet-parekh]

FWIW, flameshot never worked for me on multi-monitor wayland. It keeps misplacing my screens whenever I start flameshot. Often causing less than 50% of my display to actually be visible on the screen. Didn't test it with a single monitor though. Just in case this wasn't known. Because of that and screen-sharing issues on wayland, I always disable wayland and set xorg as default.

[borgmanJeremy]

Okay I confirmed on wayland you cannot span multiple displays with a single screenshot. I did not test on Windows, but I think this is the case on windows also.

Because I want consistent platform behavior, I am going to go ahead with my proposal to use append. It would be nice if on X we wrapped the buttons across the screens since the screenshot can span the screens, but it's alot of effort to maintain two implementations especially since X is legacy and eventually going away.

[borgmanJeremy]

@jeet-parekh See if #1081 solves the issue. I need to spend a bit more time testing a code reviewing but it seems to be directionally correct. (obviously need to pull out the debug statements)

[jeet-parekh]

Sure. I'll test it and let you know in a couple of days.

[jeet-parekh]

@borgmanJeremy, I would be testing this today. I remember you mentioning about running it in a VM earlier. Should I run #1081 on a VM? Or would it be fine directly on my laptop?

[mmahmoudian]

@jeet-parekh I believe VM was the easiest to reproduce the issue for @borgmanJeremy , but you should try it on you laptop.

[jeet-parekh]

1081 did not fix it for me.

I'm not familiar with C++ build tools, so I'll also post the commands I executed. Let me know I'm doing something wrong.

git clone https://github.com/borgmanJeremy/flameshot
cd flameshot/
git checkout origin/toolbar_fix
mkdir build
cd build/
cmake -DCMAKE_INSTALL_PREFIX=/home/jeet/dev/flameshot/final_build ..
make
make install
cd /home/jeet/dev/flameshot/final_build/bin/
./flameshot launcher

[borgmanJeremy]

Okay thanks for the feedback, you built it correctly. I'll look at it again, must be another issue.

[borgmanJeremy]

I cannot make the issue appear again with this fix. Just to rule out issues with you compiling it could you try the app image generated by the CI: Here is a direct link and some screenshots for finding it: https://we.tl/t-C2wZiLfPSn

[jeet-parekh]

That link didn't work. So I used the appimage from the artifacts zip. And the issue still remains.

$ sha256sum ./Flameshot-0.8.5.x86_64.AppImage
f630ca5f6d3f6e596da3540b2a918da00b62545592ee05c68577233fcdce618a  Flameshot-0.8.5.x86_64.AppImage

$ ./Flameshot-0.8.5.x86_64.AppImage launcher

I don't know what exactly was supposed to change by your fix. But from your earlier analysis... I am still able to take a screenshot like this one.

Also, while taking that second screenshot, this happened.

[jeet-parekh]

@borgmanJeremy, just a suggestion... if this issue is tricky to fix, would it be possible to add a hotkey to toggle the visibility of the buttons? Something like how spacebar works right now.

P.S. I just upgraded to Fedora 33. Flameshot still doesn't work for me on multi-monitor wayland. But that's less of an issue, since I always set xorg as defaut.