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

Bug 1329003: improve diff experience #532

Merged
merged 1 commit into from Jan 23, 2017
Merged

Conversation

BychekRU
Copy link
Contributor

@BychekRU BychekRU commented Jan 18, 2017

@mathjazz Don't you think that comparing previous translations with next will look more logically?
Before:
2017-01-18_02-20-49
After:
2017-01-18_11-06-19

@mathjazz
Copy link
Collaborator

@mathjazz mathjazz commented Jan 18, 2017

Thanks for the patch! I see your point and since you're not the first one with this idea, I'd like to open a discussion. ;)

Some context - we'd like to figure out how to implement suggestion diffs. There are (at least) two possible approaches.

  1. Make suggestion diffs against active translation.
  • It's the first translation listed in the History tab, also displayed in the sidebar and textarea.
  • It's usually the approved translation and if no such translation exist, it's the newest suggestion.
  • This is the approach currently deployed to production.
  • See the "Before" screenshot above.
  • Only @mathjazz supports this approach. :) Maybe also @tomer, who filed bug 1329003?
  1. Make suggestion diff against the first translation submitted.
  • It's the last translation listed in the History tab, unless it's approved - then it's first.
  • It's always the oldest suggestion.
  • This is the approach proposed in this PR.
  • See the "After" screenshot above.
  • @jotes @TheoChevalier @BychekRU are in favour of this approach.

Also adding @flodolo @MikkCZ @mastizada @StoyanDimitrov @stasm @zbraniecki @Pike @gueroJeff.

I'm also thinking that we should make the diff display optional, because it's sometimes more useful to see the actual translation (especially if the change is relativelly big). Maybe display diffs by default and the actual translation on hover (or vice versa). Or have a toggle in settings (I'd like to avoid cluttering the UI with non-essential features, so a button for each suggestion would be an overkill).

Thoughts?

@tomer
Copy link
Contributor

@tomer tomer commented Jan 18, 2017

I'd also suggest that when a translation is completely different than the other translation, there is no point in providing differences. It can be done by counting the number of changed characters or words, and if it is above a defined threshold - a diff should not be provided.

default

@flodolo
Copy link
Collaborator

@flodolo flodolo commented Jan 18, 2017

As Tomer wrote, option 1 makes the suggestion almost unreadable if there are a lot of differences compared to the current translation.

I'm not sure I understand option 2: if there's a different translation stored, why would it be useful to diff against the oldest one, which is already irrelevant?

Personally I would like option 1 (diff against the current stored translation, none if no translation is submitted yet), with a switch to show the string or the diff.

I don't think it would clutter the UI too much, and I would prefer not to hide that in Settings: I normally touch them once per session, e.g. to enable suggestions, I would like to enable/disable the diff string by string if I can't spot immediately what is changing.

BTW, 👍 for the diff, I've always wanted that.

@stasm
Copy link
Contributor

@stasm stasm commented Jan 18, 2017

Comparing against the oldest, original translation sounds like an approach which won't scale well over time. For this reason I'd support a change to always diff against the most recent approved translation.

I'm also not sure if I understand the screenshots well: is the diff displayed inside of the older translation? ( the diff is between a from and a to revision, by older I mean from.) In the Before screenshot in #532 (comment), the green text seems to denote obsolete parts of the translation which were removed in the new iteration. If that's the case, I'd advise changing this too. Could we show the diff inside of the to revision? It should probably be optional so that it doesn't clutter the most recent revision by default.

@tomer
Copy link
Contributor

@tomer tomer commented Jan 18, 2017

I don't think it would clutter the UI too much, and I would prefer not to hide that in Settings

What if diff view would be visible only on mouse hover or something alike?

@tomer
Copy link
Contributor

@tomer tomer commented Jan 18, 2017

I think that both options could be useful, and I'll add a third option - diff against the source string, which would be useful for non en-us English locales, as well as everyone else when source string changes without key change, and when translating long strings with some untranslatable keywords.

I'd suggest that the base point for the diff would be left for the user to choose, like how it is dealt on Wikipedia and the diff on each locale sign-off page.

@flodolo
Copy link
Collaborator

@flodolo flodolo commented Jan 18, 2017

What if diff view would be visible only on mouse hover or something alike?

I would prefer something that doesn't change without an explicit action on my part.

@StoyanDimitrov
Copy link
Contributor

@StoyanDimitrov StoyanDimitrov commented Jan 18, 2017

This is a must-have! The readability will improve greatly if diff-by-character is replaced with diff-by-word, given the language is not German (longer words, jk). I've been there (not Germany).

@flodolo
Copy link
Collaborator

@flodolo flodolo commented Jan 18, 2017

Example of how diff can be detrimental, I basically can't read the original sentence

schermata 2017-01-18 alle 13 48 08

@Pike
Copy link
Collaborator

@Pike Pike commented Jan 18, 2017

Yeah, there are two problems in that screenshot, one is that the diff is too fragmented. diff-match-patch has options there, IIRC. (Options I don't use on elmo yet, either)

And the styling can be made less harsh.

Also I slowly start to understand what the patch actually did. On elmo, we show both strings with highlights on what's new and what's changed. It might be an option to not show the removed text at all.

@MikkCZ
Copy link
Contributor

@MikkCZ MikkCZ commented Jan 18, 2017

I agree with stasm, that the diff should be against the last approved revision. Or maybe against what is currently was loaded in the input (but not changing dynamically when writing there).

Tomer has also point, that when the diff is too big (Levenshtein distance lev(rev1, rev2) > X ?) the diff highlighting makes it only worse to read.

@MikkCZ
Copy link
Contributor

@MikkCZ MikkCZ commented Jan 18, 2017

@Pike or maybe just highlight somehow the place where some change happened? I like Pontoon kind of minimal UI, but the colors break it a bit and a lot of attention to the history. I have asked my friend, what does she think this is, and she thought that the colors mean some feedback from the translation manager (green what is 👍 and red what is wrong).

@mastizada
Copy link
Contributor

@mastizada mastizada commented Jan 18, 2017

I like current implementation: showing changes on old translation. But using diff on words instead of letters will be a lot better as it is not readable.

pontoon-diff

@mathjazz
Copy link
Collaborator

@mathjazz mathjazz commented Jan 18, 2017

All, thank you very much for the extensive feedback!

There are two groups of issues pointed out in your comments:

1. What should we diff against? There's a pretty strong consensus we should keep making diffs against the currently active translations. No action needed here.

2. Readability. It sucks, especially if diff is relatively big. Action items:

  • Make use of diff-match-patch options to increase human readability.
  • Investigate options for more gentle styling.
  • Add ability to turn diffs on and off for each suggestion.

I'll get back to you as soon as I address the action items.

@stasm
Copy link
Contributor

@stasm stasm commented Jan 19, 2017

Am I the only one who finds it weird that the diff is displayed in the from revision and that the green text actually means "this was removed in the subsequent revision"?

@mathjazz
Copy link
Collaborator

@mathjazz mathjazz commented Jan 19, 2017

Am I the only one who finds it weird that the diff is displayed in the from revision and that the green text actually means "this was removed in the subsequent revision"?

@stasm The diff is displayed within each revision. Obviously that means there's no diff to be displayed for the base revision, which is the active translation - listed on top of the History tab.

For each revision, we run diff(base_revision, this_revision). So the green text in each revision represents the text that needs to be added to the base_revision in order to get to this_revision.

Does that make more sense? If not, what would?

(BTW, we should probably keep the chronological order in the History tab instead of moving the active revision on top.)

@BychekRU BychekRU force-pushed the bug-1329003 branch 2 times, most recently from bb4c65e to 1eed04f Compare Jan 19, 2017
@mathjazz
Copy link
Collaborator

@mathjazz mathjazz commented Jan 19, 2017

So @BychekRU and I have made 4 changes:

  • The diffs are now less fragmented.
  • Styling of changed text uses some more contrast.
  • There's a Show/Hide diff togle located above each suggestion. Sadly there's no familiar diff icon we could use.
  • Not sure about this one, needs more real-life testing: diffs are hidden by default.

You can test the changed on stage:
https://mozilla-pontoon-staging.herokuapp.com/sl/appstores/whatsnew/whatsnew_android_45.lang/?string=149465

Feel free to add or remove translations in any of the projects and locales on the staging server - it doesn't push to repositories.

Thoughts?

@mastizada
Copy link
Contributor

@mastizada mastizada commented Jan 19, 2017

Is it possible to show complete words when there are more than 2-3 changes in the same word? It will improve readability but if it is too complex then no problem.

PS. Show diff icon: can be placed near to approve, delete icons with similar to git icon, which will mean versions.
pontoon-diff
diff

@mathjazz mathjazz force-pushed the bug-1329003 branch 2 times, most recently from 6ab8a54 to 2752045 Compare Jan 20, 2017
@BychekRU
Copy link
Contributor Author

@BychekRU BychekRU commented Jan 21, 2017

Output with using diff_cleanupSemantic()
2017-01-21_17-23-16 2
Output with diff_cleanupEfficiency()
2017-01-21_17-24-57
Now I use this two functions together:
2017-01-21_17-58-47

* Use diff-match-patch options for better readability
* Hide diff by default and use toggle to turn it on and off
@mathjazz mathjazz changed the title fixed bug 1329003, add hints, fix some links Bug 1329003: improve diff experience Jan 23, 2017
Copy link
Collaborator

@mathjazz mathjazz left a comment

Thanks for the patch @BychekRU and thanks for the feedback everybody.

Merging changes for improved readability via diff-match-patch options and diff toggle.

@mathjazz mathjazz merged commit b95fcdc into mozilla:master Jan 23, 2017
1 check passed
@mathjazz mathjazz deleted the bug-1329003 branch Jan 23, 2017
@stasm
Copy link
Contributor

@stasm stasm commented Jan 23, 2017

For each revision, we run diff(base_revision, this_revision). So the green text in each revision represents the text that needs to be added to the base_revision in order to get to this_revision.

Does that make more sense? If not, what would?

@mathjazz Commenting here a bit late, sorry. Thanks for the explanation. I was a bit confused at first by the fact that the green color was used to show text removed in the active translation. I'm used to diffs showing that in red.

However, after browsing Pontoon for a while I see that it makes sense for the use-case. For instance https://pontoon.mozilla.org/fr/firefox-for-ios/firefox-ios.xliff/?string=66899 is very clear to me, so everything seems fine!

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.

None yet

9 participants