Page MenuHomeSoftware Heritage

Added explanatory tooltips on the pictos for the revision history
ClosedPublic

Authored by faux on Apr 7 2021, 11:37 AM.

Details

Summary

When browsing the history of revisions, some revisions are shown with a tag lable, other with a commit label, and there is no clear meaning shown.

For readability, added explanatory tooltips that explain what each picto means.

Closes T1751

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build has FAILED

Patch application report for D5432 (id=19428)

Rebasing onto fe53dbee8e...

Current branch diff-target is up to date.
Changes applied before test
commit 3b78452232fe5a7d98078bf117005bff651e327e
Author: Mihir Karbelkar <karbelkar.mihir@gmail.com>
Date:   Wed Apr 7 14:34:33 2021 +0530

    Added explanatory tooltips on the pictos for the revision history

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/650/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/650/console

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 7 2021, 11:44 AM
Harbormaster failed remote builds in B20477: Diff 19428!

Looks good apart some string format syntax.

Also you need to upate tests in swh/web/tests/browse/views/test_origin.py.
The simplest way will be to check that the string title="The releaseis present in the HTML output
as many times there is releases displayed (see example here).

The test that is currently failing is not related to your changes, it is due to the freshly released docutils 0.17
that generates different HTML output compared to its previous version, I will fix the issue.

swh/web/browse/snapshot_context.py
1369–1373

Please use Pyhon 3.7 f-strings instead of that old formatting syntax, this is what we use now in our codebase.

1397

same here.

Sure, I followed the syntax for formatting the string from another file. I will make the changes, and add a test for the tooltip and push.

The test that is currently failing is not related to your changes, it is due to the freshly released docutils 0.17
that generates different HTML output compared to its previous version, I will fix the issue.

D5435

Once landed you will have to rebase the diff on origin/master and update it.

Changed old formatting to f-strings and updated tests

faux marked 2 inline comments as done.Apr 7 2021, 2:07 PM

Build is green

Patch application report for D5432 (id=19448)

Rebasing onto ebc729aebe...

Current branch diff-target is up to date.
Changes applied before test
commit 3630b1968f9d3a9602fddacaf5a331be85a62508
Author: Mihir Karbelkar <karbelkar.mihir@gmail.com>
Date:   Wed Apr 7 14:34:33 2021 +0530

    Added explanatory tooltips on the pictos for the revision history

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/653/ for more details.

faux requested review of this revision.Apr 7 2021, 2:14 PM

Looks good !

Please add yourself in the CONTRIBUTORS file in a separate commit.

I will accept the diff afterwards.

Looks like your previous commit did not get included.

Use arc diff --update D5432 origin/master to include the two commits in your branch.

CONTRIBUTORS
9 ↗(On Diff #19450)

Please add your name in lexicographical order.

This revision now requires changes to proceed.Apr 7 2021, 2:34 PM

Build is green

Patch application report for D5432 (id=19450)

Could not rebase; Attempt merge onto ebc729aebe...

Updating ebc729ae..f56bd1df
Fast-forward
 CONTRIBUTORS                              | 1 +
 swh/web/browse/snapshot_context.py        | 9 +++++++++
 swh/web/templates/browse/releases.html    | 2 +-
 swh/web/tests/browse/views/test_origin.py | 1 +
 4 files changed, 12 insertions(+), 1 deletion(-)
Changes applied before test
commit f56bd1dfc010f3c6e76a425db285d637cf1d1b41
Author: Mihir Karbelkar <karbelkar.mihir@gmail.com>
Date:   Wed Apr 7 17:54:54 2021 +0530

    CONTRIBUTORS: Add Mihir Karbelkar

commit 3630b1968f9d3a9602fddacaf5a331be85a62508
Author: Mihir Karbelkar <karbelkar.mihir@gmail.com>
Date:   Wed Apr 7 14:34:33 2021 +0530

    Added explanatory tooltips on the pictos for the revision history

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/654/ for more details.

  • Added explanatory tooltips on the pictos for the revision history
  • CONTRIBUTORS: Add Mihir Karbelkar
faux marked an inline comment as done.Apr 7 2021, 2:43 PM

Oh, I am more used to the PR way where we can send away the new commits to a PR :) . Have pushed both commits.

Build is green

Patch application report for D5432 (id=19452)

Rebasing onto ebc729aebe...

Current branch diff-target is up to date.
Changes applied before test
commit 0e1a506460b8e8dd6f0f765e535ed856c893b985
Author: Mihir Karbelkar <karbelkar.mihir@gmail.com>
Date:   Wed Apr 7 17:54:54 2021 +0530

    CONTRIBUTORS: Add Mihir Karbelkar

commit 3630b1968f9d3a9602fddacaf5a331be85a62508
Author: Mihir Karbelkar <karbelkar.mihir@gmail.com>
Date:   Wed Apr 7 14:34:33 2021 +0530

    Added explanatory tooltips on the pictos for the revision history

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/655/ for more details.

Oh, I am more used to the PR way where we can send away the new commits to a PR :) . Have pushed both commits.

Yep, Phabricator is quite different from other forges for PR. Hopefully, we should migrate to GitLab soon.

Thanks for tackling this, you can now merge your feature branch to the master one and push it to origin (use git, not the Phabricator Web UI).

$ git checkout master
$ git merge T1751-Tooltip
$ git push origin master
This revision is now accepted and ready to land.Apr 7 2021, 2:49 PM