Show tag changes in object history view#6448
Show tag changes in object history view#6448iandees wants to merge 7 commits intoopenstreetmap:masterfrom
Conversation
|
Apologies, I thought I had run linting successfully. I'll tackle the failing CI checks. |
|
Unchanged tags look slightly odd with the white bar of a different width |
Agreed. What do you think about adding a grey bar of the same width? 
For the record, I suggested the change to the border color (rather than the whole cell or row) in an attempt to avoid having to deal with text on odd color backgrounds. |
pablobm
left a comment
pablobm
left a comment
There was a problem hiding this comment.
I love this ❤️ Some notes:
- The border in the middle (
border-startof value cell) looks perhaps a bit too thin to me, particularly when contrasted with the one in the left (tag name cell). Probably not a big deal, but I wonder if other variants can be experimented. - I see you added a background color for dark mode. To me it looks subtle enough not to clash with the text, so that's good. Still I wonder: is there a rationale for this difference?
- I wonder if there's a way to make this work with assistive technologies too. An
aria-*tag or something, but I can't find anything right now and it's a very tricky subject anyway. Perhaps for a future PR.
I was experimenting but didn't mean to include this in the PR. I'll remove it.
I'm happy to explore this, but I agree that it should probably be another PR.
Here's a variant where the whole row is highlighted in the color: 
Highlight the row with a stronger left border: 
Here's one where just the value part of the table is highlighted: 
|
|
This is a smart design, makes it a lot easier to find changes at a glance. My first impression is that we could probably even diff within a changed tag, word by word like MediaWiki does, for more clarity. But we can always fine-tune the presentation separately.
Along these lines, we should probably supplement the colors with some other visual distinction, because the traffic light colors aren’t great for colorblind friendliness. We already use these colors in the change counts in a changeset listing, but the user can depend on the position to distinguish between additions and deletions. Here the user can depend on deletions to be italicized and changes to have a “→”. But nothing distinguishes an added tag from an unchanged value. Not sure if that’s a problem with these particular colors. We should track colorblind friendliness in a separate issue if it isn’t trivial to deal with in this PR. |
|
Red/green colorblindness is definitely a thing, so what I have is suboptimal from that standpoint. What about using the arrow to show something like "not present → " and " → not present", with "not present" in italics? It looks quite a bit visually noisier though: 
|
|
We could use the git/diff wording. We even could get rid of the arrow and be more like diff: amenity cafe
- name oldname
+ name newname
+ opening_hours sometimes
- fixme add opening timeBut please keep the color (green/yellow/red or (green/red depending on implementation). |
|
I like the idea overall! Some small notes
Yeah, we will need some non-colour indications here
It's worth noting that not all languages have italics (e.g. Chinese) so we can't rely on italics to differentiate between user-generated tag values and system-generated UI text. And I like the variations with the strong border and more gentle backgrounds. I wouldn't be surprised if the strong red background is too close to the blue link colour to pass the accessibility contrast checks, but I haven't actually checked the numbers on this. |
|
I like the gentle backgrounds too 👍 For colour-blindness, another option would be using 
|
pablobm
left a comment
pablobm
left a comment
There was a problem hiding this comment.
Still looking into how to better resolve the duplication in browse/_tag_details, but these are some quick notes.
|
|
|
I gave it an attempt at refactoring: iandees/openstreetmap-website@highlight-tag-changes-in-history...pablobm:openstreetmap-website:highlight-tag-changes-in-history History is clean and you can read each commit separately. Thoughts? |
That does look significantly better, thanks for taking the time to do that. Mind if I cherry pick your suggestions onto my branch? |
|
Definitely, do as you please 👍 My branch was only an experiment. |
|
Ok, I think the code is probably in a decent spot now. I'm going to fiddle with trying to improve the visualization a bit now. |
|
I added +/- for value changes. It's getting a little tight in there, especially when the value is a long string like a URL. It think it helps with vision impairments though: 
|
But why adding the +- to the value cell? |
See conversation above. We are looking for something to show the change without relying solely on color. |
hacked a bit in devTools: |
|
That looks better, but it's harder to distinguish between a tag's value changing and a tag getting added. Maybe I'll change it so the key stays yellow but the value gets the red/green -/+ treatment. |
In that case, I’ve seen some sites like Apple Developer use |
|
I've spent the evening stretching the limits of my CSS knowledge and getting fairly close to what I'm going for: 
I'm not happy with this yet.
More later! |
|
I switched to using an extra 
Happy to take feedback on this before spending much more time fixing tests. |
|
It looks great! But the plus and minus signs for value change looks confusing to me. Wouldn't it be better to use tilde ( |
Than you would loose the info which is old an which is new. |
|
Ok, the tag changes are wrapped in |
|
Could we try and clean up the commit history here... I've pretty sure there aren't really 33 separate units of change here and some of them are very obviously fixup commits that should be merged into the commit they are fixing. |
Yep, absolutely. |
iandees
force-pushed
the
highlight-tag-changes-in-history
branch
from
b00f93a to
08fb505
Compare
pablobm
left a comment
pablobm
left a comment
There was a problem hiding this comment.
I think this is ok. The initial refactor and the markup-generating code make things difficult to follow, but it can be followed a bit better when going commit-by-commit.
If anything, the markup generation could be untangled a bit. I gave it a shot at iandees/openstreetmap-website@highlight-tag-changes-in-history...pablobm:openstreetmap-website:highlight-tag-changes-in-history and I think it's an improvement there. Not sure if it worth pursuing elsewhere, after playing with it a bit more.
tomhughes
left a comment
tomhughes
left a comment
There was a problem hiding this comment.
In addition to the specific comments could you rebase this please as there are apparently some conflicts.
Having now tried this after reviewing the code I think a lot of my comments may be explained by the fact that this only works in the history list, so I guess current_version is not the current version in the normal sense but is the version currently being rendered?
I have to say I find it a bit odd that the changes aren't shown when looking at a specific version - it actually took me a while to find out how to trigger this to the extent that I was putting in debugging code to try and find out why it didn't seem to be working!
| previous_version = all_versions | ||
| .find_index { |v| v.version == current_version } | ||
| &.then { |index| index.positive? ? all_versions[0...index].reverse : nil } | ||
| &.find { |v| !v.redacted? || params[:show_redactions] } |
There was a problem hiding this comment.
This is trying to cope with current_version not existing in all_versions but when would that happen? Not only would I expect it to always exist but I'd expect it to always be at the end? In which case this should work:
| previous_version = all_versions | |
| .find_index { |v| v.version == current_version } | |
| &.then { |index| index.positive? ? all_versions[0...index].reverse : nil } | |
| &.find { |v| !v.redacted? || params[:show_redactions] } | |
| previous_version = all_versions | |
| .reverse | |
| .drop(1) | |
| .find { |v| !v.redacted? || params[:show_redactions] } |
| tag.ins(format_value(key, change_info[:current], :skip_wikidata_preview => true)) | ||
| ] | ||
| when :removed | ||
| tag.del(format_value(key, change_info[:previous] || "", :skip_wikidata_preview => true)) |
There was a problem hiding this comment.
When will a removed key not have a previous value? As far as I can see wrap_tags_with_version_changes always includes it/
| tag.del(format_value(key, change_info[:previous] || "", :skip_wikidata_preview => true)) | ||
| when nil | ||
| # For unknown versioning show the current value with the wikidata preview | ||
| format_value(key, change_info[:current] || "", :skip_wikidata_preview => false) |
There was a problem hiding this comment.
Same here for the current value? As far as I can see it should always have one?
| # Generate single row for other change types | ||
| cells = [tag.th(format_key(key), :class => "py-1 border-secondary-subtle table-secondary fw-normal")] | ||
|
|
||
| # Only add diff indicator cell if we're in a history/diff context | ||
| cells << tag.td(get_change_indicator_text(change_info[:type]), :class => get_indicator_cell_class(change_info[:type])) if change_info[:type] | ||
|
|
||
| cells << tag.td(format_tag_value_with_change(key, change_info), :class => "py-1") |
There was a problem hiding this comment.
Why break this down into multiple statements that append to a temporary array when the modified version just does it all inline?
|
Some musing about next steps following this PR:
I experienced this too, but I think it’s a symptom of a broader issue. The history and version pages have distinct headings, but the tops of the pages still look roughly the same. I think moving the breadcrumb and pagination bar to the top of the page would address this confusion. It would be more convenient now that the two pages contain substantially different content (diffs on one, wiki previews on the other). I also wonder if we should have a separate page for diffing two specific versions. Most of the time, when I link to a single version, the snapshot in time is more interesting than the changes from a previous version. But sometimes I want to call attention to the changes affecting a particular element in a particular changeset. Or I want to compare the cumulative effect of multiple revisions in a row, for example to determine whether a revert was complete or not. To be clear, I don’t think these ideas would block the current PR by any means. |
iandees
force-pushed
the
highlight-tag-changes-in-history
branch
2 times, most recently
from
bbb1948 to
80826fd
Compare
iandees
force-pushed
the
highlight-tag-changes-in-history
branch
from
80826fd to
556e834
Compare
|
Alrite, I think I addressed review comments and this is ready for another look. |
tomhughes
left a comment
tomhughes
left a comment
There was a problem hiding this comment.
A couple of overall comments...
Firstly, please don't create "address review feedback" commits but instead fold the changes into the original commit(s) where the code being changed was introduced.
Secondly, maybe it's just me but to me the way this shows changes seems to be backwards to me... Consider this example:
That way was leisure=park in v1 and changed amenity=school to in v2 but it looks like the change is happening in v1, which is logically impossible and hence why I spotted it. Then you realise it's actually showing the change backwards as well at which point it makes a bit more sense but is assuming you're reading backwards in time from the present version.
Maybe it's just the programmer in me and normal people expect it to work like that but it's not what a programmer used to reading diffs expects to see!
| <% key = tag[0] %> | ||
| <% version_details = tag[1] %> | ||
| <% rows = format_tag_row_with_change(key, version_details) %> | ||
| <% rows.each do |row| %> | ||
| <%= row %> | ||
| <% end %> |
There was a problem hiding this comment.
You kind of have to ask, once every line in your view is a ruby interpolation, is it serving any purpose as a view partial?
Given this is the only use of format_tag_row_with_change maybe that function should incorporate the rest of the logic here so there's just one interpolation?
Then once you've done that maybe the partial goes away altogether and we just call that helper in the one partial that currently invokes this partial, either iterating the collection there or changing the helper again to process the whole collection?
|
|
||
| previous_tags = previous_version&.tags || {} | ||
|
|
||
| tags_added = tags_modified = tags_unmodified = tags_removed = tags_with_unknown_versioning = {} |
There was a problem hiding this comment.
I should have realised this earlier:
a = b = {}
a[:foo] = 1
a # => {foo: 1}
b # => {foo: 1}(By the way, this is the same in other languages. I just checked in Python and JS).
Therefore, the assignment needs to be split:
| tags_added = tags_modified = tags_unmodified = tags_removed = tags_with_unknown_versioning = {} | |
| tags_added = {} | |
| tags_modified = {} | |
| tags_unmodified = {} | |
| tags_removed = {} | |
| tags_with_unknown_versioning = {} |
This doesn't solve the issue that @tomhughes found in a separate comment (incorrect changes assigned to each versions). Still looking into that one.
There was a problem hiding this comment.
In fact, I think this doesn't really matter in the end because each variable is re-assigned rather than mutated. But still good practice.
| def wrap_tags_with_version_changes(tags_to_values, _current_version = nil, all_versions = []) | ||
| # Find the previous usable version by looking backwards from the end | ||
| previous_version = all_versions | ||
| .reverse | ||
| .drop(1) | ||
| .find { |v| !v.redacted? || params[:show_redactions] } |
There was a problem hiding this comment.
I think the problem (as mentioned by Tom) is here. We never use _current_version, so we always get the same version as previous_version. All comparisons are to... is it the second to latest version? Whichever.
I think this is the correct code:
| def wrap_tags_with_version_changes(tags_to_values, _current_version = nil, all_versions = []) | |
| # Find the previous usable version by looking backwards from the end | |
| previous_version = all_versions | |
| .reverse | |
| .drop(1) | |
| .find { |v| !v.redacted? || params[:show_redactions] } | |
| def wrap_tags_with_version_changes(tags_to_values, current_version = nil, all_versions = []) | |
| # Find the previous usable version by looking backwards from the end | |
| usable_versions_in_order = | |
| all_versions | |
| .reverse | |
| .filter { |v| !v.redacted? || params[:show_redactions] } | |
| current_version_index = usable_versions_in_order.find_index { |v| v.version == current_version } | |
| previous_version = current_version_index && usable_versions_in_order[current_version_index + 1] |
|
I wanted to mention that I tried to solve the same but went with a different approach, see #6995. If mine doesn't make it, it might be of interested anyway. |
|
Honestly, I prefer my approach a lot more. It's way simpler, clear, doesn't break and especially doesn't confuse with existing. The diff shouldn't be in the state. The diff is the diff in between states. It's whats changed, not what is. Having the diff in the state at a given time makes it more ambiguous whether we look forward or backward. It's something to look into, not something that must be visible at all times. Further, having just both values above/below each other makes it clearly comparable by eye. Lastly, the KISS bootstrap colors fit all existing schemas. It looks like this in the existing view and once expanded using no js it's all in there. 
|
12 participants
Description
This adds highlighting of tag changes in each version to the history view for primitives:
old → newdeletedScreenshot:
This probably closes #738. I know there are other discussions and tickets related to this idea, but I couldn't find them in my few searches.
Ruby is not my native language, so please point out where I can improve the change.
How has this been tested?
I ran the site locally in Docker, added some fake data to the database, and inspected it on my browser locally.