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.

Sign up for GitHub

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

Jump to bottom

Crowdin v2 fixes #641

Closed
wants to merge 4 commits into from
Closed

Crowdin v2 fixes #641

wants to merge 4 commits into from

Conversation

berezins
Copy link
Contributor

@berezins berezins commented Jul 1, 2020

Fixed some problems/bugs found during testing on all platforms as well as reported by @vslavik by email

@berezins
Fix getting proj and file IDs from filename
e382123
@berezins
Fix upload to Crowdin on Save when opened locally
d7f17d7 
from cache or PO downloaded in browser via Crowdin UI
@berezins
Fix crash on saving PO downloaded in browser
eb61e67 
- At least on VS2019. Due the copy of `projects` `vector` is captured by lambda while the original `projId` belongs to - is destroyed after upper function returns, while lambda is was called asynchroneously later and attemted to use captured `projId` (which already belonged/pointed to non-existent `vector`)
- Arrange/refactor variables captures/scopes to only where they actually
  used (to decrease chance to use some variable in not proper
  place/lambda or just simplify reading of code so it clearly which
  variable where really used thanks to limiting it scope)
@berezins berezins mentioned this pull request Jul 1, 2020
Closed
@vslavik
Copy link
Owner

vslavik commented Jul 1, 2020

Fix getting proj and file IDs from filename

Sorry for having introduced that; it is fixed as a side-effect of c53547e.

@vslavik
Copy link
Owner

vslavik commented Jul 2, 2020

Fix upload to Crowdin on Save when opened locally … d7f17d7

See #638 — I'm having doubts about whether Poedit should sync on save.

@vslavik
Copy link
Owner

vslavik commented Jul 2, 2020

Fix crash on saving PO downloaded in browser

Finally this part is outdated based on #633 (comment), as we can just remove that code, no?

@berezins
Use X-Crowdin-Project-ID and X-Crowdin-File-ID
43870fc 
in PO instead  of `X-Crowdin-Project` (project `identitifier`) and `X-Crowdin-File` (full path to file in Crowdin)
@berezins
Copy link
Contributor Author

berezins commented Jul 2, 2020

Fix upload to Crowdin on Save when opened locally … d7f17d7

See #638 — I'm having doubts about whether Poedit should sync on save.

It worked same way in v2.3 Poedit and IMO it's properly/useful (why not? what disadvantages it might cause for users?)

@berezins
Copy link
Contributor Author

berezins commented Jul 2, 2020

Done using of X-Crowdin-Project-ID and X-Crowdin-File-ID in 43870fc

@berezins
Copy link
Contributor Author

berezins commented Jul 2, 2020

Fix crash on saving PO downloaded in browser

Finally this part is outdated based on #633 (comment), as we can just remove that code, no?

Yep, pls feel free to remove that outdated commit from this branch/PR or let me know so I would remote it on my own

@vslavik
Copy link
Owner

vslavik commented Jul 2, 2020

It worked same way in v2.3

I don't think that's true - this was the only AttachCloudSync use

IMO it's properly/useful (why not? what disadvantages it might cause for users?)

  • the linked bug you discovered
  • failure to sync presents as failure to save
  • confusing UI with two buttons doing the same (see the bug)
  • inconsistent behavior between opening-from-crowdin and opening-from-recent-files

@vslavik
Copy link
Owner

vslavik commented Jul 2, 2020

Use X-Crowdin-Project-ID and X-Crowdin-File-ID

Non-conflicting commit as 79e1f92 — thanks!

@berezins
Copy link
Contributor Author

berezins commented Jul 2, 2020 via email

@vslavik
Copy link
Owner

vslavik commented Jul 2, 2020

Which one bug?

#638 repeatedly referenced above.

Isn't it confusing even more?

Yes, as I said, the UI is confusing and I need to figure out what to do. Which is why I won't be introducing any new inconsistent behaviors until I figure it out.

The current state is no worse than 2.3. Changes here can wait until after 2.4.

@berezins
Copy link
Contributor Author

berezins commented Jul 2, 2020 via email •
edited
Loading

@vslavik
Copy link
Owner

vslavik commented Jul 3, 2020

but it's definitely not
good as well so making Save just save locally (and never upload) would
make it quite consistent IMO and would solve this problem quite quickly and
easily for a while for 2.4 I think.

Yep. There are actually four cases and I failed to consider them all:

  1. Open from Crowdin, edit, sync back.
  2. Open from recent files as a shortcut for 1., with intent to sync to Crowdin.
  3. Open a manually downloaded file, with intent to sync to Crowdin.
  4. Open a file that happens to have Crowdin metadata, with no desire to sync.

Case 4 is e.g. when opening Poedit's own PO files — they are downloaded from Crowdin, they report as being sync-capable. But I absolutely do not want my changes when playing with the files to sync. They happen to be sync friendly, but I didn't download them from Crowdin manually for that purpose.

So Save behavior in these cases is:

  1. Save could be said to be expected to sync, hence the motivation for having it that way (it does now)
  2. It's OK to sync on save.
  3. It's OK to sync on save.
  4. Save MUST NOT sync.

And then there are 4 more cases, same as above with "...but while being offline" appended.

So I don't see any other safe and consistent way to do it in 2.4 other than not syncing on Save at all, as you suggest. What I am worried about is that it is non-obvious for previous users, or even for new ones, that Save is local-only. There should be some indicator of # of not-yet-uploaded changes, or a prompt telling to sync on closing (or saving) the file, or something.

@berezins
Copy link
Contributor Author

berezins commented Jul 3, 2020

So I don't see any other safe and consistent way to do it in 2.4 other than not syncing on Save at all, as you suggest. What I am worried about is that it is non-obvious for previous users, or even for new ones, that Save is local-only. There should be some indicator of # of not-yet-uploaded changes, or a prompt telling to sync on closing (or saving) the file, or something.

You definitely better then me know expectations of Poedit customers, but just sharing my own expirience with Poedit about what I tried to guess intuitively when used Poedit and tried to figure out Save behaviour/logic initially

So it's just IMO the best quickest fix for 2.4. would be either

  • Never upload on Save, i.e. in none of cases 1, 2, 3/4 while only Sync should be used to upload in all 1, 2, 3, 4 cases. I.e. Save behaviour would be same and consistent i all cases. I.e. just never upload.
  • Upload on Save in 1, 2, but not 3/4, while only Sync should be used in case of 3/4 to upload. Which should look pretty logical and intuitive as well since when customer expect file to be uploaded back when opened via Open from Crowdin - IMO it's logically and intuitively to expect it to be uploaded back in case it opened from local cache (Recent) (since customer aware that it has been opened via Open from Crowdin before as well).

@vslavik vslavik mentioned this pull request Jul 4, 2020
Closed
@vslavik
Copy link
Owner

vslavik commented Jul 4, 2020

You definitely better then me know expectations of Poedit customers

Expectations are best learned and clarified through conversations like this, which I deeply appreciate...

Upload on Save in 1, 2, but not 3/4, while only Sync should be used in case of 3/4 to upload.

I think you're right! That indeed seems to be where we should draw the line. I'll make a separate PR for this (together with addressing #638 as this is all related) and ask you to look it over if you don't mind.

@vslavik vslavik mentioned this pull request Jul 4, 2020
Merged
@vslavik
Copy link
Owner

vslavik commented Jul 9, 2020

I think this one is outdated now, right?

@berezins
Copy link
Contributor Author

berezins commented Jul 9, 2020

I think this one is outdated now, right?

Yep, if you mean this branch/PR

@vslavik vslavik closed this Jul 12, 2020
berezins added a commit to berezins/poedit that referenced this pull request Jul 12, 2020
@berezins
Fix no sync on Save after explicit Sync
98c4a17 
- If file opened via `Open from Crowdin` or from cache
- Also as side effect to the case 3/4 of vslavik#641 (comment)
  (but IMO it's really very useful feature/addition)
  if `Sync` button was pressed at least once in above case - it means that customer intetned to
  sync that file with Crowdin so all futher `Save` will upload that file
  to Crowdin
berezins added a commit to berezins/poedit that referenced this pull request Jul 12, 2020
@berezins
Fix no sync on Save after explicit Sync
0cb32f0 
- If file opened via `Open from Crowdin` or from cache
- Also as side effect to the case 3/4 of vslavik#641 (comment)
  (but IMO it's really very useful feature/addition)
  if `Sync` button was pressed at least once in above case - it means that customer intetned to
  sync that file with Crowdin so all futher `Save` will upload that file
  to Crowdin
berezins added a commit to berezins/poedit that referenced this pull request Jul 12, 2020
@berezins
Fix no sync on Save after explicit Sync
c4a2a9a 
- If file opened via `Open from Crowdin` or from cache
- Also as side effect to the case 3/4 of vslavik#641 (comment)
  (but IMO it's really very useful feature/addition)
  if `Sync` button was pressed at least once in above case - it means that customer intetned to
  sync that file with Crowdin so all futher `Save` will upload that file
  to Crowdin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@berezins @vslavik