Fossil Forum

New feature: dynamic loading of diff context
Login

New feature: dynamic loading of diff context

(1) By Stephan Beal (stephan) on 2021-09-11 17:32:03 [source]

The current trunk now, on JS-aware browsers, alters the diff-viewing pages to include buttons to load extra lines of context. This is based roughly on github's similar feature, which is exemplified at:

https://github.com/msteveb/autosetup/commit/235925e914a52a542

This diff demonstrates it pretty well:

https://fossil-scm.org/home/vinfo/49a60a580de9f3c2

Because this feature requires a connection to the server, it does not get applied to the CLI-side output of the diff -b and -by options. Likewise, on non-JS-aware browsers it doesn't do anything: the diffs will appear like they always have, with a placeholder in the slot where the new UI controls are injected.

It will certainly undergo some tweaking but is essentially feature-complete. This is a brand new feature and is bound to have some bugs and feature requests. As always, i'll make every attempt to squash (or justify) any bugs and justify away any feature requests ;). (Even so, please submit them!)

Known issues:

  • Bootstrap skin does "something weird" with the diff tables in at least one of its mediaquery (screen size) configurations, applying inconsistent sizes, while behaving properly in others. The other skins are believed to be working as expected.

(2) By sean (jungleboogie) on 2021-09-12 00:29:14 in reply to 1 [link] [source]

Okay, I have another suggestion for consideration.

Within the context section of a commit, put the up/down arrows. https://fossil-scm.org/home/info/0f7740b6329e5338

As of now, you can click the check-in link to traverse the timeline while in diff viewing mode, but might be neat to see more commits.

(3) By Stephan Beal (stephan) on 2021-09-12 01:11:56 in reply to 2 [link] [source]

Within the context section of a commit, put the up/down arrows.

That is an interesting idea. That would require a new remote API but shouldn't pose any particular problem. That said, i have multiple TODOs lined up already so won't immediately start on that one.

(4) By sean (jungleboogie) on 2021-09-12 15:22:19 in reply to 1 [link] [source]

FYI…

On mobile, the slightly smaller font remains slightly smaller when going up the diff, but the font size increases going down the diff. Also when viewing down, the line numbers don’t match up with the font.

(5.1) By Stephan Beal (stephan) on 2021-09-12 17:21:40 edited from 5.0 in reply to 4 [link] [source]

Also when viewing down, the line numbers don’t match up with the font.

i can reproduce that on mobile but not the desktop. It's independent of the new loading feature, though: it happens even without loading new content.

i'm looking into a solution but debugging mobile quirks like this is a bit of a pain.

It's easily visible in the bottom half of this chunk.

(6) By Stephan Beal (stephan) on 2021-09-12 16:46:16 in reply to 5.0 [link] [source]

i'm looking into a solution but debugging mobile quirks like this is a bit of a pain.

It's reproducible on both FF and Chrome on Android, but all attempts to solve it so far have not done so. We had this same problem with the line numbering reimplementation and the solution was to ensure that the font sizes and line heights were all forcibly propagated down the DOM. That fix is not working here.

Unfortunately, it's not reproducible in Chrome's dev tools using mobile device emulation, so it's about impossible to debug exactly what's mismatched here. The best we can do, unless someone has a clear idea of what's wrong, is trial and error until it works.

i'm looking into remote debugging options.

(7) By Stephan Beal (stephan) on 2021-09-12 17:53:11 in reply to 4 [link] [source]

... the line numbers don’t match up with the font.

That's now fixed on trunk. Thank you for the report!

Details: Larry and Richard noticed that the misalignment started with lines which contained Unicode arrows (which is why default.css demonstrated it so clearly). That observation made the solution obvious: increase the line-height. We just had to tinker with that value until we found a workable minimum.

(8) By sean (jungleboogie) on 2021-09-12 19:41:55 in reply to 7 [link] [source]

Private mail sent with a picture of the issue I’m seeing.

(9) By sean (jungleboogie) on 2021-09-13 14:15:40 in reply to 7 [link] [source]

Now that I'm back on a computer, here's the picture I sent to Stephan. We've exchanged a few emails over the weekend and he's been unable to see what I continue to see, even after the fix.

https://imgur.com/a/t3peRN8

Does anyone else see this layout on their mobile device? I'm using an iphone 6 with Firefox installed. I've deleted cache/private data and I've tried on multiple different wifi networks and tried on my mobile network.

I'm not so concerned about me not having the proper layout, but I'd rather be sure it's only me and not a surprise once there's a new release.

(10.1) By Stephan Beal (stephan) on 2021-09-13 18:40:03 edited from 10.0 in reply to 9 [link] [source]

Does anyone else see this layout on their mobile device? I'm using an iphone 6 with Firefox installed.

i'm still unable to reproduce that on both a tablet and phone with FF and Chrome, but i have a suspicion. Is it possible for you to test a patch from your phone? If so, please add this to the end of src/default.css:

table.diff tr, table.diff td,
table.diff td > pre,
table.diff td > pre > * {
  font-size: inherit;
  line-height: inherit;
}

That "shouldn't be necessary, but i suspect that the built-in style for that particular browser is overriding the font size and/or line-height for the elements via CSS rules which are more specific (better matches) than ours.

Edit: if you're unable to try that via your phone, let me know and i'll check that in for testing purposes. It won't have any effect on most clients but adds CSS we don't (or shouldn't) strictly need.

The font-size thing you reported in another thread is a separate issue i haven't yet looked into but have a suspicion about (with an easy solution if it turns out to be correct).

(11.1) By sean (jungleboogie) on 2021-09-13 19:32:02 edited from 11.0 in reply to 10.1 [link] [source]

I made the changes to default.css on top of the latest Fossil commits and started my fossil server on the Fossil repo.

The most recent commit displays and expands correctly, but selecting other random commits causes the misalignment that only I have seen.

The eagle and bootstrap skins seem to have the similar issue, if that means anything to you.

This is a diff of the changes:

Index: src/default.css
==================================================================
--- src/default.css
+++ src/default.css
@@ -1917,5 +1917,12 @@
 @media screen and (max-width: 1200px) {
   .wideonly {
     display: none;
   }
 }
+
+table.diff tr, table.diff td,
+table.diff td > pre,
+table.diff td > pre > * {
+         font-size: inherit;
+           line-height: inherit;
+    }

(12) By Stephan Beal (stephan) on 2021-09-13 20:06:02 in reply to 11.1 [link] [source]

The most recent commit displays and expands correctly, but selecting other random commits causes the misalignment that only I have seen.

The misalignment "should" only show up on checkins which include certain parts of default.css, as that's the only file which includes the characters which trigger the problem. For a reproducible case, see the bottom half of this chunk.

One more thing to try is to change this block in default.css:

table.diff pre {
  margin: 0 0 0 0;
  padding: 0 0.5em;
  line-height: 1.20 /* important for mobile:
                      see forum post e6f4ee7de98b55c0 */;
}

and increase that 1.2 to 1.3 or even 1.4. You will probably see blank gaps between the insertion/deletion lines with that change, but it might fix the misalignment (and i might have a way of filling those gaps - i'll try that while you're trying this).

(13) By Stephan Beal (stephan) on 2021-09-13 20:26:15 in reply to 12 [link] [source]

and increase that 1.2 to 1.3 or even 1.4.

If 1.3 or 1.4 works for you, please try incrementally smaller values until you find where it breaks. The lower the value, the better, but 1.2 is the current minimum which we know works in most places, so don't go below that. If 1.3 works for you, i'd start next with 1.25.

i seem to have found a way to eliminate the color gaps which will appear with higher line-height values, which i'll apply if we can find that a given line-height value works for your browser.

(14) By Florian Balmer (florian.balmer) on 2021-09-19 11:23:39 in reply to 1 [link] [source]

The fossil.diff.js code is quite sophisticated, and I think it's really well done! Congratulations, nice work!

Here's my accumulated feedback from combing through the code in detail while backporting it to my IE-compatible version of Fossil.

(0) Backporting is surprisingly easy: replace const with var, prepend the replacement code for fossil.dom.js (see below) to the script, and rewrite the fetchArtifactChunk() routine to handle all the XHR stuff itself (about 20 lines of code).

var D = {
addClass:function(e){for(var i=1;i<arguments.length;i++)e.classList.add(arguments[i]);return e;},
append:function(p){for(var i=1;i<arguments.length;i++)p.appendChild(arguments[i]);return p;},
attr:function(e){if(arguments.length==2)return e.getAttribute(arguments[1]);for(var i=1;i<arguments.length-1;i+=2)e.setAttribute(arguments[i],arguments[i+1]);return e;},
clearElement:function(e){while(e.firstChild)e.removeChild(e.firstChild);return e;},
div:function(p){var e=document.createElement('div');if(p)p.appendChild(e);return e;},
pre:function(p){var e=document.createElement('pre');if(p)p.appendChild(e);return e;},
remove:function(e){e.parentNode.removeChild(e);return e},
span:function(p){var e=document.createElement('span');if(p)p.appendChild(e);return e;},
td:function(p){var e=document.createElement('td');if(p)p.appendChild(e);return e;}
};

Together, the dom and fetch rewrites amount to less than 1.5 KB of Javascript code to replace two 28 KB and 9 KB Javascript framework libraries. ;-)

(1) It looks like the fetchTrChunk() function in fossil.diff.js is unused, so at least (almost) 4 KB that could be stripped. ;-)

(2) On a desktop monitor, the "Cannot load chunk while a load is pending" (toast? notification?) message is not only (too) far away from the proceedings, but is usually (after a bit of scrolling to get to the diffs) blending over the "Context" area, which already has different background colors -- so it regularly gets unnoticed. I also think the message is unnecessary, and would like to suggest it to be replaced with some disabled-state look of the up/down buttons (for example, with CSS similar to the hover-state look, but in the opposite direction of brightness).

(15) By Stephan Beal (stephan) on 2021-09-19 22:41:26 in reply to 14 [link] [source]

Together, the dom and fetch rewrites amount to less than 1.5 KB of Javascript code to replace two 28 KB and 9 KB Javascript framework libraries. ;-)

You're ignoring the caching and reuse effects, however: fossil.fetch and fossil.dom are used in all of the JS-heavy pages and have an extremely low amortized traffic cost. Also, code cost is measured not only in size but in developer/development time. Those extra kb are an insignificant cost compared to the time it takes me to write in plain DOM API code (and re-write it for every page/app).

addClass:function(e){for(var i=1;i<arguments.length;i++)e.classList.add(arguments[i]);return e;},

You've removed an important feature there which is actually used by some of our apps (multiple target DOM elements as the first argument). The fossil.dom functions are written they way they are because the person writing the client code finds their features useful ;).

If we did not have library-level code in place to eliminate stuff like DOM node creation and manipulation, most of our JS code would never get written because it's simply too tedious to develop any significant amount of JS that way. Any amount of time spent rewriting features costs me more than the double-digit kilobytes of reuse do.

It looks like the fetchTrChunk() function in fossil.diff.js is unused, so at least (almost) 4 KB that could be stripped. ;-)

i'll get that removed in a few moments - it was an early failed development direction.

On a desktop monitor, the "Cannot load chunk while a load is pending" (toast? notification?) message is not only (too) far away from the proceedings, but is usually (after a bit of scrolling to get to the diffs) blending over the "Context" area...

That's a fair point - a toast was used because i needed a solution and didn't (and still don't) have a more suitable one readily available except for simply sending the errors to console.error with no UI interaction.

Disabling the buttons would be overkill, though. The errors, if any, are very likely transient network problems. In all of development and testing, the only time the error toasts showed up was when there was a bug in the related code, not due to a network problem or XHR error response. If you were seeing errors frequently, it's possibly because you were working on the code?