[l10nstats] indicate which locales change when in tree history

VERIFIED FIXED

Status

Webtools
Elmo
P5
normal
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Pike, Assigned: adrian)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

Follow up from bug 773478:

One thing I'll drop in the first iteration is a marker on which locales change in the tree history plot.

This is an OK semi-regression, as for fast cycles, the timeplot-based graph showed way to many blue sparks, which made manouvering the histogram hard.

One idea is to just overlay the graph with transparent boxes with a tooltip, and to join boxes that are too close to each other. Then do some hover/js magic.
(Reporter)

Updated

6 years ago
Priority: -- → P5
(Reporter)

Comment 2

5 years ago
Let's put this up for grabs.
Assignee: m → nobody
I've been trying to parse what it is you think it should do. 

I'm guessing you hover over the graph (the red or green area) and a tooltip appears that follows the house that includes the locales at that point in time in the tooltip.
Is that near? 

You can currently click the graph but how it works I think only Axel knows. It updates the histogram below but it's a mystery what it actually represents and hopefully I don't need to know. 

What I don't understand is "join boxes" means? What boxes? What does "join" mean?

Also, do we want this tooltip to appear on hover or on clicks? If we do it on hover, considering how many datapoints there are in a graph, I suspect the browser would really struggle to keep up if at every pixel we have to loop through LOCALE_DATA each time.
(Assignee)

Comment 4

5 years ago
"join boxes" means, when there are several small changes for the same day, merge them as one and avoid the little pixel-y ladder-like things.
Assignee: nobody → adrian
(Assignee)

Comment 5

4 years ago
Notes from today's discussion with Axel:

- show an HTML tooltip that the user can click
- make each locale a link to this page: https://l10n.mozilla.org/dashboard/history?[parameters]
- separate locales depending on their states (show "good" and "gray" locales lists)
(Assignee)

Comment 6

4 years ago
Created attachment 8421690 [details]
locales_changes_2-1.png
(Assignee)

Comment 7

4 years ago
Created attachment 8421691 [details]
locales_changes_2-2.png
(Assignee)

Comment 8

4 years ago
Created attachment 8421694 [details] [diff] [review]
patch to add a new layer on the graph and tooltip boxes with links for each changing locale

This is a first patch for this bug, with all the UI part. It adds a new layer to the graph, containing only white boxes that are displayed on mouse over, and shows a tooltip box with a list of all changing locales. Each locale is a link to the history page of the current tree and locale. 

I still need to modify the data so we have fewer points in the graph.
Attachment #8421694 - Flags: feedback?(l10n)
(Reporter)

Comment 9

4 years ago
Comment on attachment 8421694 [details] [diff] [review]
patch to add a new layer on the graph and tooltip boxes with links for each changing locale

Review of attachment 8421694 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch.

I've applied the patch locally, and played with it a bit.

I like the overall display of the popup, and the left-right switch magic. I think we should relabel "shady" to "OK-ish", though.

I was expecting the area-overlay to be a rolling one, as in, it'd be continuously following the cursor as it hovers over. I'm not exactly sure how static regions will work out, I think I should give that a try.

I'd max out the width of the areas, though, if there's a bunch of nothing in the graph, let's not show the popup.

Also, I think the area should be going to both the left and right of a change, right now it's only going to the right.

As usual with UX reviews, I didn't dive too much into the code itself.
Attachment #8421694 - Flags: feedback?(l10n) → feedback+
(Assignee)

Comment 10

4 years ago
Created attachment 8428756 [details] [diff] [review]
Improved locales data layer + joined boxes

This is a new patch, but this time I think it is complete. I added some server-side code to merge the locales data when two events are too close to one another, and I have addressed your comments from the previous feedback. 

(In reply to Axel Hecht [:Pike] from comment #9)
> I was expecting the area-overlay to be a rolling one, as in, it'd be
> continuously following the cursor as it hovers over. I'm not exactly sure
> how static regions will work out, I think I should give that a try.

That would be a very different behavior from what I have done so far, and would need a lot more work from me. Hopefully with this new patch you won't feel the need for this anymore! :D
Attachment #8428756 - Flags: review?(l10n)
(Assignee)

Comment 11

4 years ago
Created attachment 8428757 [details]
locales_changes_3-1.png
Attachment #8421690 - Attachment is obsolete: true
Attachment #8421691 - Attachment is obsolete: true
Attachment #8421694 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 8428758 [details]
locales_changes_3-2.png
(Assignee)

Comment 13

4 years ago
Created attachment 8428759 [details]
locales_changes_3-3.png
Comment on attachment 8428756 [details] [diff] [review]
Improved locales data layer + joined boxes

Review of attachment 8428756 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, but this is not the direction I have for this graph. For others, I'll attach a visual comparison before and after the current patch.

Using the squashed data for the whole graph is not what I want, there's value in how the micro-progress happens. The idea is to show these graphs to people who don't understand the progress of localizations, and showing them the squashed data just misleads.

I found the flickering bar with the static areas tricky to navigate, too.

Yep, I know, loads of work, sorry. I obviously didn't get what you intended when you said

> I still need to modify the data so we have fewer points in the graph.

in comment 8.

I think we should do this purely in the html/js of the page.

Finding the changes is relatively straightforward, here's some code that I twiddled out in a scratchpad:

function findChanges(starttime, endtime) {
  var i = loc_data.length;
  var rv = {};
  while (--i >= 0) {
    var ldata = loc_data[i];
    if (ldata.time < starttime) {
      break;
    }
    if (ldata.time >= starttime && ldata.time <= endtime) {
      for (var loc in ldata.locales) {
        if (!rv.hasOwnProperty(loc)) {
          rv[loc] = _getState(ldata.locales[loc]);
        }
      }
    }
  }
  return rv;
}

Probably want to invert the return value still, but you get the idea.

Maybe we should get on a call to dive into the details of the behaviour to avoid running into the same situation we're in right now?

::: apps/l10nstats/static/l10nstats/js/tree_progress.js
@@ +60,4 @@
>  var loc_data = LOCALE_DATA;
>  var locales = [];
>  
> +dashboardHistoryUrl += "&starttime=" + formatRoundedDate(startdate) + "&endtime=" + formatRoundedDate(enddate) + "&locale="

jshint: not declared variable, missing trailing semicolon, also, we should only set the endtime if the original graph has an endtime.

::: apps/l10nstats/views.py
@@ +233,5 @@
>  
> +    # We don't want to show too much data, because that makes the graph
> +    # hard to read and manipulate. So we make sure that not more than
> +    # 100 data points are displayed.
> +    delta_full_range = (endtime - starttime).total_seconds()

python2.6 doesn't have that, there's a workaround in the progress command already.
Attachment #8428756 - Flags: review?(l10n) → review-
Created attachment 8438512 [details]
"wireframe"

Here's how I think we should show the graph, and summarize the changes within a time window. Does that help?
(Assignee)

Comment 16

4 years ago
Created attachment 8470896 [details] [diff] [review]
patch-773612-locale-changes.diff

New version of this patch, this time I believe we are close! Axel, what do you think?
Attachment #8428756 - Attachment is obsolete: true
Attachment #8428757 - Attachment is obsolete: true
Attachment #8428758 - Attachment is obsolete: true
Attachment #8428759 - Attachment is obsolete: true
Attachment #8470896 - Flags: review?(l10n)
Comment on attachment 8470896 [details] [diff] [review]
patch-773612-locale-changes.diff

Review of attachment 8470896 [details] [diff] [review]:
-----------------------------------------------------------------

I know that I had a problem with the tooltip, but I can't reproduce it. I guess if we would use just CSS for the expand/collapse display, that'd go away?

Anyway, this is the right thing, and performs great, but I have a few things on the code side.

::: apps/l10nstats/static/l10nstats/js/tree_progress.js
@@ +198,5 @@
> +      var ldata = loc_data[i];
> +      if (ldata.time > endTime) {
> +        break;
> +      }
> +      else if (ldata.time >= startTime && ldata.time <= endTime) {

no else after break, that makes the code easier to read.

Also, you're double-checking endTime here.

I'd call _getState(ldata.locales[loc]) once here, and then split the code path.

@@ +242,5 @@
> +      shady: []
> +    };
> +    for (var l in localesInRange) {
> +      triagedLocales[localesInRange[l]].push(l);
> +    }

I'm wondering if we should move this code into findChanges?

@@ +247,5 @@
> +
> +    // Finaly show those locales in the tooltip box.
> +    goodLocalesElt.empty();
> +    badLocalesElt.empty();
> +    shadyLocalesElt.empty();

These should just be in addLocalesToElement, maybe rename that to showLocalesInElement to reflect that?

@@ +287,5 @@
> +    $('.clipped', this).toggle();
> +  }).on("mouseout", function () {
> +    hideTooltip();
> +    $('.clipped', this).toggle();
> +  });

The clipped logic seems a bit too complex, can we just make that happen via :hover selectors? Default CSS to show ellipsis and hide the extra locales, :hover CSS to hide the ellipsis and show the extra locales?
Attachment #8470896 - Flags: review?(l10n) → feedback+
(Assignee)

Comment 18

4 years ago
Created attachment 8476700 [details] [diff] [review]
patch-773612-locale-changes.diff

New version of this patch, ready for a (final?) review! A diff with the precedent patch is coming up shortly.
Attachment #8470896 - Attachment is obsolete: true
Attachment #8476700 - Flags: review?(l10n)
(Assignee)

Comment 19

4 years ago
Created attachment 8476701 [details] [diff] [review]
Diff from last patch

The diff from the previous entire patch, to show only what changed since the last review.
Comment on attachment 8476700 [details] [diff] [review]
patch-773612-locale-changes.diff

Review of attachment 8476700 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet, thanks. r=me
Attachment #8476700 - Flags: review?(l10n) → review+

Comment 21

4 years ago
Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/f3e9e4e3ac220c91353d387b0bb95491239ceeb7
Fixes bug 773612 - Show a tooltip with the changing locales in the tree-history graph. r=Pike

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Yay me for complaining about things and then reviewing them, and not grokking that they're not working.

I'll land a follow-up on this, so that we can actually move this to production.

(ldata[loc] outside of the loc loop isn't cool)

Comment 23

4 years ago
Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/b96c196d0823dc9063f288a5ea5fd095e15bc8bd
bug 773612, rearrange the for loop so that loc is defined when used. rs=me, bustage
With my follow-up, this works on dev. Good for a cut to prod now.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.