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

New Feature: now flameshot has ability to show screenshot history #3619

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Michaelzhouisnotwhite
Copy link

Usage

use ', ' and '. ' to get history of screenshots:

  • ',' for showing history backward
  • '.' for showing history forward

Details:

My contribution

add a singleton class: CaptureHistoryUtils

history screenshots that user export are saved in getCachePath() + /cap.his.<selection size> + <datetime>

ScreenShots

flameshot.mp4

@Michaelzhouisnotwhite Michaelzhouisnotwhite marked this pull request as ready for review May 28, 2024 07:04
@Michaelzhouisnotwhite Michaelzhouisnotwhite marked this pull request as draft May 28, 2024 07:15
@Michaelzhouisnotwhite Michaelzhouisnotwhite marked this pull request as ready for review May 28, 2024 07:17
@mmahmoudian
Copy link
Member

Thanks for the PR. I truely appreciate it.

In general we would like to have a PR after the issue is already discussed in an issues or in our Matrix channel. This would help the maintainers to see if they think the feature is in-line with the perspective of the project, and also would help during the review process.

Apart from that, I'm extremely puzzled on what this feature does. I quickly checked the code and that didn't solve much of my confusion. Would you please elaborate on:

  1. What is the use-case of this feature
  2. How a user should use this

To me it seems you clearly have a need for such feature and you were kind enough to also push the feature to the main repo. I just don't understand what that need was.

@Michaelzhouisnotwhite
Copy link
Author

Michaelzhouisnotwhite commented Jun 4, 2024

Thanks for the PR. I truely appreciate it.

In general we would like to have a PR after the issue is already discussed in an issues or in our Matrix channel. This would help the maintainers to see if they think the feature is in-line with the perspective of the project, and also would help during the review process.

Apart from that, I'm extremely puzzled on what this feature does. I quickly checked the code and that didn't solve much of my confusion. Would you please elaborate on:

  1. What is the use-case of this feature
  2. How a user should use this

To me it seems you clearly have a need for such feature and you were kind enough to also push the feature to the main repo. I just don't understand what that need was.

Thanks for your time to review this pr.

1. What is the use-case of this feature

This feature is to be able to view the pictures that have been captured in the past. I call it back-tracking screenshot history.

For example, when I want to screenshot and copy a live screen, but the captured screen is not perfect as the screen is no longer exists. I can use this function to backtrack to the previous screen and capture the area again.

2. How a user should use this

a. open the flameshot
b. type ',' or '. ' on the keyboard to see the history of screenshots

main function implement is here:
image

@mmahmoudian
Copy link
Member

Thanks for the explanation. I can see that it can be useful for some users. I also like the name you chose for it (back-tracking).

In the light of Microsoft Windows Recall and the similarities to this back-tracking feature, I have a few concerns:

  1. User should be able to toggle it on/off, and by default it should be off (such features with potential privacy implications should be Opt-in)
  2. User should be able to define how much cache they would like to keep, and the rest should be cleaned up (I think number of screenshots is better than a Megabyte cutoff, so the default I guess should be a relatively small number like 5)
  3. Would be super nice (end even essential) that the screenshots in the cache folder can be encrypted somehow and only decrypted when needed. This is something that I don't have a clear solution for it. For example:
    • Flameshot create a password-protected ed25519 pgp key pair for asymmetric encryption. The keys can be saved in the config folder of Flameshot
    • when screenshot is taken and this back-tracking feature is enabled, the public key is used to encrypt the screenshot and then saved to the cache folder. For this part, no input from user is needed as the public key does not need password to encrypt
    • when user wants to use the back-tracking to bring the previous screenshots from the cache, we can ask the user once for the password to decrypt the screenshots on the fly and show them
    • when user is done with back-tracking and closes the window of flameshot, then flameshot "forgets" the password

You can create a new tab in the config window (flameshot config) for back-tracking settings. There can be:

  1. a checkbox to toggle this feature
  2. a part for the user to define cache folder path
  3. a button to generate the pgp key pairs for the user (if the key already exists, the generate button should be disabled and a text should be be added to indicate the existence of a key)

About the encryption part we need to discuss this with some more folks and get their opinion on the best practices, but for now if you can add the tab in the config to address my first two concerns, that would be great.

In the meantime, we can also wait for the following folks to weigh in and express their opinions:
@veracioux @panpuchkov @jack9603301 @hosiet @holazt @lupoDharkael

@Michaelzhouisnotwhite
Copy link
Author

Michaelzhouisnotwhite commented Jun 4, 2024

You can create a new tab in the config window (flameshot config) for back-tracking settings. There can be:

  1. a checkbox to toggle this feature
  2. a part for the user to define cache folder path
  3. a button to generate the pgp key pairs for the user (if the key already exists, the generate button should be disabled and a text should be be added to indicate the existence of a key)

About the encryption part we need to discuss this with some more folks and get their opinion on the best practices, but for now if you can add the tab in the config to address my first two concerns, that would be great.

Thanks for your suggestions. In the near future,I will complete the following needs:

  1. a checkbox to toggle this feature
  2. a part for the user to define cache folder path
  3. a spinbox to define a number of screenshot need to keep in cache.

footnote: this 'back-tracking' idea comes from anothor closed-source screenshot application named Snipaste. However, the Snipaste only avaliable on windows. Flameshot is a great app and opensource, so I want the flameshot has this feature as well. So this pr came up.

- add backtracking path settings
- add backtracking cache limits
- add backtracking enable button
@mmahmoudian
Copy link
Member

@Michaelzhouisnotwhite Thank you for your hard work. I highly appreciate it.

May I suggest going through the code and use a unified naming scheme for this feature? I put a small comment on one of your commits, but now that I went through the whole PR again, I realized that this is potentially a wider concern.

In the code we have these diverse terminologies which are more or less pointing to the same concept

  • "backtrack[er]" (sometimes "backTrack")
    • E.g. the file name src/config/backtrackingimpl.cpp
  • "rollback" (sometimes "rollBack")
    • E.g. inside the file src/config/backtrackingimpl.cpp
  • "btk"
    • E.g. inside the file src/config/backtrackingimpl.cpp
    • E.g. inside src/config/backtrackingimpl.h
  • "screen history"
    • E.g. inside src/widgets/capture/capturewidget.cpp
  • "History"
    • E.g. inside src/widgets/capture/capturewidget.h lines 78-81
  • "capturehistory"
    • E.g. file name of src/utils/capturehistoryutils.cpp
      • inside src/utils/CMakeLists.txt

I Understand that they are not all identical, but I suggest trying to unifying the terminology to make the maintenance of the project easier in the future. I believe the "backtrack" is a very fitting name and less confusing comparing to "rollback" or "history", because these pictures (as far as I understand them) are raw PNGs (no annotation, no cropping...).

Anyways, I feel this is a nice PR. Thank you again.

@stuart938503
Copy link

While this PR wouldn't meet the original request in #240, it is related (IMO) and helps address the essence of the problem in that issue - editing screenshots after the capture window is closed.

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.

3 participants