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

Added the ability to cache the last region #2615

Merged
merged 3 commits into from
Jun 1, 2022
Merged

Conversation

borgmanJeremy
Copy link
Contributor

No description provided.

@borgmanJeremy
Copy link
Contributor Author

@mmahmoudian I still need to review the code a bit more before merging but this adds an option that enables re-using the last region if you want to test it .

@mmahmoudian
Copy link
Member

I have few points to raise, so I enumerate them to be able to easily discuss them:

  1. I don't think this is a correct approach to have this enabled by default. At least I find it very disturbing that every time I click on the tray icon the same position is selected. I think the better approach is to have one of the following:
    1.1. a checkbox in the configurations to be able to toggle this feature
    1.2. only limit this to Launcher

I personally think the latter makes more sense as the user can also see the geometry.

I additionally suggest having the following (simultaneously):

  • a CLI argument that let the user to spawn this way (e.g --last-region). This would be useful as one-off mode and to be bound to keyboard shortcuts
  • if we go with the Launcher, we should add a checkbox to make the FLameshot ignore the geometry there. This way the user can choose how to use the Launcher.
  1. the behavior is inconsistent between when the flameshot gui is executed, and when tray icon is clicked. I believe they should behave identically to avoid confusion.

@borgmanJeremy
Copy link
Contributor Author

1.1. a checkbox in the configurations to be able to toggle this feature

This exists today, but I'm fine to make it disabled by default:

image

a CLI argument that let the user to spawn this way (e.g --last-region). This would be useful as one-off mode and to be bound to keyboard shortcuts

Good suggestion, I'll add this.

the behavior is inconsistent between when the flameshot gui is executed, and when tray icon is clicked. I believe they should behave identically to avoid confusion.

Can you describe the difference? They behave the same for me

I first had this feature confined to only the launcher, but then I added the config option that enables it globally. So the behavior is the launcher always remembers the last region, but flameshot gui / tray icon can be optionally enabled or disabled.

src/main.cpp Outdated Show resolved Hide resolved
@mmahmoudian
Copy link
Member

mmahmoudian commented May 21, 2022

This exists today, but I'm fine to make it disabled by default:

You are right, Somehow I unconsciously expected it to be added to the bottom of the list of checkboxes. My bad.

Can you describe the difference? They behave the same for me

On my computer, when I run flameshot gui it will not respect the new config checkbox (i.e same behavior as before), but when I click on the tray icon it does.

So the behavior is the launcher always remembers the last region, but flameshot gui / tray icon can be optionally enabled or disabled.

I'm fine with this. Sounds logical to me, specially if we get that new CLI argument, users will be much less dependent on the launcher for this feature in one-off mode. So in my mind, this is perfect 👍

@borgmanJeremy
Copy link
Contributor Author

@mmahmoudian are you aborting the screenshot without copying to clipboard or saving? I only save the previous region when a final action happened. I did all my testing on MacOS but the gui cli option is behaving the same as the tray. I do further testing on Linux.

@borgmanJeremy borgmanJeremy merged commit 46801d9 into master Jun 1, 2022
@borgmanJeremy borgmanJeremy deleted the region_rework branch June 1, 2022 14:19
@mmahmoudian
Copy link
Member

Sorry, I forgot to answer. Yeah I was cancelling. We should clarify in our documentation that this happens only when the final action is triggered.

panpuchkov pushed a commit to namecheap/flameshot that referenced this pull request Jun 12, 2022
* Added the ability to cache the last region

* adding cli option

* addressed typo comments and applied clang-format

(cherry picked from commit 46801d9)
@edarague
Copy link

Thank you very much, it is a feature that many people were clamoring for... finally!! Now I will just use FlameShot. 🎉🥳

@Adolio
Copy link

Adolio commented Jul 5, 2022

Thanks a lot 😀 That's awesome! 🎉

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.

4 participants