flameshot-org / flameshot

Powerful yet simple to use screenshot software :desktop_computer: :camera_flash:
https://flameshot.org
GNU General Public License v3.0
24.62k stars 1.58k forks source link

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

Open jeet-parekh opened 3 years ago

jeet-parekh commented 3 years ago

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.

Screenshot from 2020-10-15 12-17-35

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 commented 3 years ago

@jeet-parekh does it happen on both monitors?

jeet-parekh commented 3 years ago

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

mmahmoudian commented 3 years ago

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

jeet-parekh commented 3 years ago

@mmahmoudian, yes. This is on 1280x1024.

Screenshot from 2020-10-15 14-36-01

borgmanJeremy commented 3 years ago

Is fractional scaling enabled?

jeet-parekh commented 3 years ago

@borgmanJeremy, nope.

borgmanJeremy commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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.

Screenshot from 2020-10-17 11-21-21

borgmanJeremy commented 3 years ago

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: 2020-10-17_13-42

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. 2020-10-17_13-47

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. 2020-10-17_13-50

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

jeet-parekh commented 3 years ago

@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 commented 3 years ago

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

jeet-parekh commented 3 years ago

1081 did not fix it for me.

Screenshot from 2020-10-24 00-34-29

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 commented 3 years ago

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

borgmanJeremy commented 3 years ago

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

image

jeet-parekh commented 3 years ago

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

Screenshot from 2020-10-28 10-08-45

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.

image

Also, while taking that second screenshot, this happened.

Screenshot from 2020-10-28 10-23-37

jeet-parekh commented 3 years ago

@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.

funnym0nk3y commented 3 years ago

With 0.9.0 this issue still persists.

I'm using Win10 20H2.

fireFerry commented 3 years ago

Something similar is happening to me with using a laptop and my TV as a second monitor. Schermafbeelding 2021-03-19 090808 On my tv the buttons work normally and are properly placed outside the border. Schermafbeelding 2021-03-19 090855 if I try to take a screenshot on my main laptop screen though, the buttons move all the way in the upper left of my tv screen. this is my screen setup, 1 being my laptop screen and 2 being my tv: https://i.imgur.com/WRvGGFI.png both 1920x1080 screens.

I'm using Windows 10 20H2 with the latest patches.

mikiTesf commented 3 years ago

Related to this issue is the overlapping of the tool buttons on top of each other like this: image

This happened when I tried taking the screenshot of a small area on the bottom left corner of my second monitor. I'm running version 0.10.0.

AmandaBSobrinho commented 2 years ago

Still happening to me using a laptop with 1366x768 and an external monitor with 1920x1080p. Flameshot v0.10.1-1 on Ubuntu 20.04.

Captura de tela de 2021-10-26 12-28-30

mmahmoudian commented 2 years ago

@AmandaBSobrinho Do you have this issue if you reduce height or width of the selected region? If not, that is expected simply because you didn't give it enough room in the outside area of the selected region that Flameshot cat put those buttons.

AmandaBSobrinho commented 2 years ago

Like this? I reduced the width to less than half the screen. Is it still expected to happen? There is space above too, as you can see in the image.

Captura de tela de 2021-10-28 10-21-38

veracioux commented 2 years ago

@AmandaBSobrinho That is clearly unintended behavior. I can't reproduce this on Arch linux, X11, i3wm on a laptop with an external monitor. Tried with different scale values as well.

Does this happen on your laptop monitor or the external monitor, or both?

AmandaBSobrinho commented 2 years ago

Good to know! This only happens in the external monitor, that has the 1920p x 1080p res. In the laptop monitor, with 1366p x 768p, it behaves as it should, with the buttons being pushed outside of the box always. Is there any other info I can get here to help?

Clearly I can see, in the external monitor, that the buttons get pushed inside the selection box once I start increasing the height and it gets bigger than the laptop resolution.

borgmanJeremy commented 2 years ago

has anyone tried after #1856 was merged?

mmahmoudian commented 2 years ago

@AmandaBSobrinho

Is there any other info I can get here to help?

Would you please take a screenshot of your current monitor setup from your Ubuntu settings and also provide the output of the inxi -S -G

AmandaBSobrinho commented 2 years ago

Sure! The screenshot of the setup:

Captura de tela de 2021-10-28 11-56-25

Output of inxi -S -G:

System:    Host: ni-16321-9p Kernel: 5.11.0-38-generic x86_64 bits: 64 Desktop: Gnome 3.36.9 
           Distro: Ubuntu 20.04.3 LTS (Focal Fossa) 
Graphics:  Device-1: Intel driver: i915 v: kernel 
           Display: x11 server: X.Org 1.20.11 driver: i915 resolution: 1366x768~60Hz, 1920x1080~60Hz 
           OpenGL: renderer: Mesa Intel Xe Graphics (TGL GT2) v: 4.6 Mesa 21.0.3
j-tai commented 2 years ago

@AmandaBSobrinho Can you check if the issue still occurs on the master branch? The latest release (0.10.1) doesn't have #1856 merged yet.

AmandaBSobrinho commented 2 years ago

@j-tai Sorry, how do I get the master branch? Do I just clone it locally? I have the latest release installed via dpkg.

veracioux commented 2 years ago

@AmandaBSobrinho You can clone and compile it or you can download a pre-packaged version from here (nightly build).

AmandaBSobrinho commented 2 years ago

Well, it looks like you guys corrected it, at least here it is all looking good! Congratulations on the great job! I'll keep testing it, though, to check if I can find any situation when the bug still happens.

dandv commented 2 years ago

I still see this issue with Flameshot-0.10.2.x86_64.AppImage on a laptop+external monitor X11 setup, when the selected area is entirely on the external monitor. Video.