flameshot-org / flameshot

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

Fix full/screen capture use last region #3364

Closed Kit-p closed 9 months ago

Kit-p commented 9 months ago

Ignore "Use last region" option when running flameshot full or flameshot screen.

Closes #3086.

mmahmoudian commented 9 months ago

Thanks for the PR. I believe instead of "exclusion" criteria we should only limit this to flameshot gui. This is because the flameshot screen should also ignore this as far as I can understand the use-case.

Kit-p commented 9 months ago

Thanks for the feedback. That makes a lot of sense. In this case, it feels like we should also clarify it in the wordings. Something like "Use last region for GUI capture"?

mmahmoudian commented 9 months ago

In this case, it feels like we should also clarify it in the wordings. Something like "Use last region for GUI capture"?

Yes, you right, the current wording is sub-optimal. Would you like to also include those changes in this PR (considering that it kinda fits the context)? What needs to be changed is the config window text and it's tooltip.

Kit-p commented 9 months ago

I am not sure how the translation system works, will changing the keys invalidate all current translations?

I can include it in this PR, but the translations should be a separate issue I suppose.

Kit-p commented 9 months ago

Okay, after some digging I believe here is what I need to do. Please correct me if I misunderstand anything.

  1. Update the translation keys (English) in code.
  2. Replace the translation keys in all translation files so current translations are kept.
  3. Notify translators to help update the translations by setting type="unfinished". (?)
mmahmoudian commented 9 months ago

As far as I understand, you don't need to care about translations. All you need to do is to change the code for English keys. Then weblate will pull changes from the master branch and automatically roll it as a string to be translated in all the supported languages.

This is also my understanding. I'm relatively new to handling translation conflicts (already a messy one has happened and I'm in the process of merging the translations manually between the master and the Weblate fork). For now, I have locked the Weblate from the admin dashboard, so it is safe change anything on our side.

Kit-p commented 9 months ago

Just tried changing English in code and compile with the GENERATE_TS option. It set the current translation to have type="vanished" and added a new string to be translated. I am just wondering if it would be better to keep the current ones, because the core meaning of the wordings hasn't deviated.

Btw, thanks a lot for the translation merging work, I definitely also noticed this messy situation.

Kit-p commented 9 months ago

Looks like this will be merged in the current state. So attached is a patch for updating the translation files, feel free to ignore if it isn't helpful. fix-flameshot-use-last-region-translation.patch

veracioux commented 9 months ago

@Kit-p I've reviewed the patch file and it looks good. Although I can't apply it for some reason - it says error: translations/Internationalization_bg.ts: No such file or directory (repeated for each file in the patch).

I think you can commit the changes from the patch here, and we'll merge them all together. But let's wait for @mmahmoudian to confirm.

@mmahmoudian You mentioned a messy merge, are you talking about the Internationalization_ta.ts file between current weblate/master and master? If yes, it looks like that file effectively has no changes, but the whole file shows up as changed in the diff viewer. This is probably due to a change in windows/linux line endings. There are also some other conflicts, but not many and not especially tricky, unless I'm looking at the wrong thing.

mmahmoudian commented 9 months ago

@veracioux

You mentioned a messy merge, are you talking about the Internationalization_ta.ts file between current weblate/master and master

I think the file in question was *_fi.ts that was causing issues. This is what Weblate is reporting itself:

Rebasing (1/44) Auto-merging data/translations/Internationalization_fi.ts CONFLICT (content): Merge conflict in data/translations/Internationalization_fi.ts error: could not apply 5aa5a5c... Translated using Weblate (Finnish) hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm ", then run "git rebase --continue". hint: You can instead skip this commit: run "git rebase --skip". hint: To abort and get back to the state before "git rebase", run "git rebase --abort". Could not apply 5aa5a5c... Translated using Weblate (Finnish)

If you have the setup ready for merging please go ahead and merge. I haven't had time this week to work on it. If not, I will try to merge tomorrow.

Kit-p commented 9 months ago

@veracioux Ready for review, thanks!

veracioux commented 9 months ago

@Kit-p Merged. Thank you for your contribution!