GNOME Bugzilla – Bug 720886
Clean up diff-view.{html,css,js} and diff-view-html-builder.js
Last modified: 2015-08-06 18:32:27 UTC
Currently, the diff-view.{html,css,js} and diff-view-html-builder.js are a little messy. Also, make diff-view-html-build.js absolutely templatable.
Created attachment 264694 [details] [review] Clean up code a bit
Created attachment 264695 [details] [review] Rename to 'table_body'
Created attachment 264696 [details] [review] Close '<tr>' tag properly
Created attachment 264697 [details] [review] Use jQuery method like the rest
Created attachment 264698 [details] [review] Use '<span>' instead '<div>'
Created attachment 264699 [details] [review] Change to much suitable 'hunk_stats'
Created attachment 264700 [details] [review] Rename 'filepath' to 'file_path'
Created attachment 264701 [details] [review] Rename 'table_body' to 'file_body'
Created attachment 264702 [details] [review] Rename 'stats' to 'file_stats'
Created attachment 264703 [details] [review] Document the usage of each code block
Created attachment 264704 [details] [review] Apply 'pre-wrap' only on code
Created attachment 264705 [details] [review] Remove obsolete css styles
Created attachment 264706 [details] [review] Follow css selector convention
Created attachment 264707 [details] [review] Put related styles together
Created attachment 264708 [details] [review] Tidy the css styles a bit
Created attachment 264709 [details] [review] Move expander '<span>' to template
Created attachment 264710 [details] [review] Reduce duplicate stage button code
Created attachment 264711 [details] [review] Extract to 'exec_template' function
Created attachment 264712 [details] [review] Extract to 'diff_hunk()' function
Created attachment 264713 [details] [review] Move the wrapped html into template
Created attachment 264714 [details] [review] Replace 'FILE_BODY' at the end It rarely happens, if 'FILE_BODY' contains placeholders, it has to be the last placeholder to be replaced.
These patches can also be seen on branch 'wip/techlivezh/diff-view-clean-up'
Review of attachment 264707 [details] [review]: This commit is merely rearranging the css styles to make it more clear.
Review of attachment 264694 [details] [review]: These changes is merely cleaning up obsolete code and formating for later commits.
Review of attachment 264694 [details] [review]: ::: libgitg/resources/diff-view-html-builder.js @@ +3,3 @@ + self.postMessage({'log': e}); +} + Moving functions around doesn't make things clearer @@ -48,3 @@ { var l = h.lines[j]; - var o = String.fromCharCode(l.type); There is nothing wrong with this ::: libgitg/resources/diff-view.js @@ +26,3 @@ unstage: 'unstage', loading_diff: 'Loading diff...' + }, No need @@ -115,3 @@ -} - -function html_escape(str) Where did this go? @@ +112,2 @@ var html_builder_tick = 0; +var html_builder_worker = 0; Don't, no need @@ +155,3 @@ }, 200); + html_builder_worker = new Worker(workeruri); Don't, no need
I started to review this, but it's too much work :) Can you please simplify the patches and make them independent. Also, I don't really care much for super trivial changes, like removing a variable declaration inline (it's not making things clearer). If you can, separate the commits in a thing that changes only code style (no semantic changes), and changes that affect the behavior/outcome.
(In reply to comment #26) > Can you please simplify the patches and make them independent. I am afraid this is the smallest patch set, which is the result of lots of rebasing and comit spliting and re-organizing, I can come up with. Acctually, except these code cleaning and rearranging commits(which has no effect to the behavior, only code apprence), the reset is mostly independent. > Also, I don't really care much for super trivial changes, like removing a variable declaration inline (it's not making things clearer). If you can, separate the commits in a thing that changes only code style (no semantic changes), and changes that affect the behavior/outcome. I do believe that semantic code changes like code naming and code locating is important for future maintaince, and a well renamed and organized code is more plansant to maintain. Like you said, it trivial and don't affect the behavior/outcome, i don't see what harm it can do. And after this series changes, you will find that diff-view.{html,css,js} and diff-view-html-builder.js looks so much clean and easy to maintain. I hate commit that mixes changes together, I strongly believe each commit should do one thing only. Acctually, this is the result of lots of rebasing trying to make each commit only changes one thing, so that we know exactly what the commit has changed. You can just scape the commits that says 'clean up', and review the rest.
Review of attachment 264713 [details] [review]: There is a reason why this part does not make use of templates, which is performance. It's much slower (on large diffs) to do the template thing for each hunk. The code as it was, although not the best looking, is like that for a reason.
Before this becomes a pointless or religious discussion, my final cents: (In reply to comment #27) > (In reply to comment #26) > > Can you please simplify the patches and make them independent. > > I am afraid this is the smallest patch set, which is the result of lots of > rebasing and comit spliting and re-organizing, I can come up with. Acctually, > except these code cleaning and rearranging commits(which has no effect to the > behavior, only code apprence), the reset is mostly independent. I don't believe that this is the smallest. I don't need separate patches for every little line of code cleanup. Wrt independent, I'm referring for example to the patch that I reviewed, where html_escape disappeared. > > > Also, I don't really care much for super trivial changes, like removing a variable declaration inline (it's not making things clearer). If you can, separate the commits in a thing that changes only code style (no semantic changes), and changes that affect the behavior/outcome. > > I do believe that semantic code changes like code naming and code locating is > important for future maintaince, and a well renamed and organized code is more > plansant to maintain. Like you said, it trivial and don't affect the > behavior/outcome, i don't see what harm it can do. > And after this series changes, you will find that diff-view.{html,css,js} and > diff-view-html-builder.js looks so much clean and easy to maintain. Sure, but this argument goes into infinity. You can always make a trivial change, but at some point this becomes very subjective. If there is a clear cleanup, I'm all for it. But if you just rearrange some functions, then no. > > I hate commit that mixes changes together, I strongly believe each commit > should do one thing only. Acctually, this is the result of lots of rebasing > trying to make each commit only changes one thing, so that we know exactly what > the commit has changed. Yes, each commit should do only one thing. But code cleanup *is* one thing. If the only changes are aesthetics, then just bunch them together. It keeps history cleaner and makes it easier to review. > > You can just scape the commits that says 'clean up', and review the rest.
Review of attachment 264695 [details] [review]: Looks good
Review of attachment 264696 [details] [review]: Good catch
Review of attachment 264696 [details] [review]: Stupid review tool...
Review of attachment 264697 [details] [review]: ::: libgitg/resources/diff-view.js @@ +181,3 @@ } + $(content).html(event.data.diff_html); OK, just note that jQuery can induce significant overhead, although I expect here it shouldn't be too bad. It would be good to measure the impact though (on a large commit).
Review of attachment 264698 [details] [review]: ::: libgitg/resources/diff-view.css @@ +116,3 @@ } +span.stats { You can't set a width on span normally, does this still work?
(In reply to comment #38) > Review of attachment 264697 [details] [review]: > > ::: libgitg/resources/diff-view.js > @@ +181,3 @@ > } > > + $(content).html(event.data.diff_html); > > OK, just note that jQuery can induce significant overhead, although I expect > here it shouldn't be too bad. It would be good to measure the impact though (on > a large commit). Just to mainain the consistency here, we can improve the performance by completely remove jQuery and use native javascript code later.
(In reply to comment #39) > Review of attachment 264698 [details] [review]: > > ::: libgitg/resources/diff-view.css > @@ +116,3 @@ > } > > +span.stats { > > You can't set a width on span normally, does this still work? It has 'display:inline-block', yes, it works.
Review of attachment 264699 [details] [review]: ::: libgitg/resources/diff-view-html-builder.js @@ +23,3 @@ var cnew = h.range.new.start; + var hunk_stats = '<span class="hunk_stats">@@ -' + h.range.old.start + ',' + h.range.old.lines + ' +' + h.range.new.start + ',' + h.range.new.lines + ' @@</span>'; This is the header of the hunk. I don't see *any* problem with that. @@ +46,3 @@ <td class="gutter old">' + lnstate.gutterdots + '</td> \ <td class="gutter new">' + lnstate.gutterdots + '</td> \ + <td class="hunk_header">' + hunk_stats + '</td> \ Also, here you should correspondingly changed the css class
Review of attachment 264700 [details] [review]: Fine
Review of attachment 264701 [details] [review]: Fine
(In reply to comment #42) > Review of attachment 264699 [details] [review]: > > ::: libgitg/resources/diff-view-html-builder.js > @@ +23,3 @@ > var cnew = h.range.new.start; > > + var hunk_stats = '<span class="hunk_stats">@@ -' + h.range.old.start + > ',' + h.range.old.lines + ' +' + h.range.new.start + ',' + h.range.new.lines + > ' @@</span>'; > > This is the header of the hunk. I don't see *any* problem with that. > > @@ +46,3 @@ > <td class="gutter old">' + lnstate.gutterdots + '</td> \ > <td class="gutter new">' + lnstate.gutterdots + '</td> \ > + <td class="hunk_header">' + hunk_stats + '</td> \ > > Also, here you should correspondingly changed the css class This actually intentional, you will see it in the later commits.
Review of attachment 264702 [details] [review]: Fine, but do not commit until the div vs. span issue is resolved.
Review of attachment 264703 [details] [review]: This is unnecessary and does nothing to make things clearer. Comments should only be used to clarify an otherwise difficult to read part of the code. Your comments just have the variable name in them (basically) and thus do not add to the readability of the code.
Review of attachment 264704 [details] [review]: Fine, except for unrelated indent. ::: libgitg/resources/diff-view.html @@ +33,3 @@ + <!-- ${FILE_STATS} --> + <span class="file_path"><!-- ${FILE_PATH} --></span> + </td> This is unrelated
Review of attachment 264705 [details] [review]: Fine, except for the unrelated px fix ::: libgitg/resources/diff-view.css @@ +163,2 @@ div#diff div.file table tr.hunk_header td.gutter.new { + border-right: 0px; This is unrelated.
Review of attachment 264706 [details] [review]: Which convention is that?
Review of attachment 264707 [details] [review]: ::: libgitg/resources/diff-view.css @@ +127,3 @@ +div.commit .message { + white-space: pre-wrap; + margin-bottom: 5px; Just add this together
Review of attachment 264708 [details] [review]: ::: libgitg/resources/diff-view.css @@ +2,2 @@ margin: 0; + padding: 0; Trivial change, please don't @@ +24,2 @@ width: 100%; + border-collapse: collapse; Trivial change, please don't @@ +104,3 @@ } +div.commit .sha1, div.commit .date { Don't @@ +112,2 @@ text-align: right; + font-family: monospace; Don't @@ -128,2 @@ white-space: pre-wrap; - margin-bottom: 5px; Didn't 5px override 15px here? @@ +174,3 @@ display: block; + float: left; + height: 5px; Don't @@ +208,3 @@ } +span.file_path, span.hunk_stats { Don't @@ +218,3 @@ min-height: 50px; + padding-right: 10px; + border-radius: 10px 10px 10px 10px; And don't
Review of attachment 264709 [details] [review]: Fine
Review of attachment 264710 [details] [review]: Nice, go for it. Actually, I think that code got committed by mistake, since that is not functional. Still, go ahead with the patch.
Review of attachment 264711 [details] [review]: This is also a matter of performance. Whereas the replacement regex was previously built once in diff_files, now it's computed every time (which is unnecessary).
Review of attachment 264712 [details] [review]: ::: libgitg/resources/diff-view-html-builder.js @@ +28,3 @@ { + lnstate.added = 0; + lnstate.removed = 0; Don't make this part of the state @@ +37,3 @@ for (var i = 0; i < file.hunks.length; ++i) { + file_body += diff_hunk(file.hunks[i], lnstate, data); Instead, have it return added/removed from diff_hunk and add it to the local vars.
Review of attachment 264714 [details] [review]: Fine
(In reply to comment #47) > Review of attachment 264703 [details] [review]: > > This is unnecessary and does nothing to make things clearer. Comments should > only be used to clarify an otherwise difficult to read part of the code. Your > comments just have the variable name in them (basically) and thus do not add to > the readability of the code. Yes,actually just tring to separate these code block apart.
(In reply to comment #46) > Review of attachment 264702 [details] [review]: > > Fine, but do not commit until the div vs. span issue is resolved. There is no issue, span already has 'display: inline-block;' in diff-view.css
(In reply to comment #50) > Review of attachment 264706 [details] [review]: > > Which convention is that? Probably need abetter commit message, just trying to keep the selector naming consistency.
(In reply to comment #58) > (In reply to comment #47) > > Review of attachment 264703 [details] [review] [details]: > > > > This is unnecessary and does nothing to make things clearer. Comments should > > only be used to clarify an otherwise difficult to read part of the code. Your > > comments just have the variable name in them (basically) and thus do not add to > > the readability of the code. > > Yes,actually just tring to separate these code block apart. I don't think it's necessary, if you want to separate those things, then use functions (but don't).
(In reply to comment #60) > (In reply to comment #50) > > Review of attachment 264706 [details] [review] [details]: > > > > Which convention is that? > > Probably need abetter commit message, just trying to keep the selector naming > consistency. Ok, go ahead
(In reply to comment #55) > Review of attachment 264711 [details] [review]: > > This is also a matter of performance. Whereas the replacement regex was > previously built once in diff_files, now it's computed every time (which is > unnecessary). The main perpose of this is to separate the diff-view html code and the builder-worker apart. We can improve the performance by replacing the 'regex prelacing' with `$('.place-holder').each(function(){$(this).html(contents)});` like the `run_template` in diff-view.js, this way it should be faster than teh regex repalcing.
(In reply to comment #61) > (In reply to comment #58) > > (In reply to comment #47) > > > Review of attachment 264703 [details] [review] [details] [details]: > > > > > > This is unnecessary and does nothing to make things clearer. Comments should > > > only be used to clarify an otherwise difficult to read part of the code. Your > > > comments just have the variable name in them (basically) and thus do not add to > > > the readability of the code. > > > > Yes,actually just tring to separate these code block apart. > > I don't think it's necessary, if you want to separate those things, then use > functions (but don't). Will remove the comments.
(In reply to comment #63) > (In reply to comment #55) > > Review of attachment 264711 [details] [review] [details]: > > > > This is also a matter of performance. Whereas the replacement regex was > > previously built once in diff_files, now it's computed every time (which is > > unnecessary). > > The main perpose of this is to separate the diff-view html code and the > builder-worker apart. > > We can improve the performance by replacing the 'regex prelacing' with > `$('.place-holder').each(function(){$(this).html(contents)});` like the > `run_template` in diff-view.js, this way it should be faster than teh regex > repalcing. jQuery should be avoided in places where performance matters. Please measure on a large commit if your patch makes a difference performance wise or not. If it significantly halters how long it takes to render, then it should be revised. Otherwise it's fine to commit.
(In reply to comment #65) > (In reply to comment #63) > > (In reply to comment #55) > > > Review of attachment 264711 [details] [review] [details] [details]: > > > > > > This is also a matter of performance. Whereas the replacement regex was > > > previously built once in diff_files, now it's computed every time (which is > > > unnecessary). > > > > The main perpose of this is to separate the diff-view html code and the > > builder-worker apart. > > > > We can improve the performance by replacing the 'regex prelacing' with > > `$('.place-holder').each(function(){$(this).html(contents)});` like the > > `run_template` in diff-view.js, this way it should be faster than teh regex > > repalcing. > > jQuery should be avoided in places where performance matters. Please measure on > a large commit if your patch makes a difference performance wise or not. If it > significantly halters how long it takes to render, then it should be revised. > Otherwise it's fine to commit. Use `jQuery` here is just for easy expressing my thought, of course it could be done with entirely native javascrit, and getting rid of the jQuery eventually is my intention.
(In reply to comment #28) > Review of attachment 264713 [details] [review]: > > There is a reason why this part does not make use of templates, which is > performance. It's much slower (on large diffs) to do the template thing for > each hunk. The code as it was, although not the best looking, is like that for > a reason. This commit is actually the main purpose for this patch set, the previous commits are just preparing for this commit to look clear.
(In reply to comment #67) > (In reply to comment #28) > > Review of attachment 264713 [details] [review] [details]: > > > > There is a reason why this part does not make use of templates, which is > > performance. It's much slower (on large diffs) to do the template thing for > > each hunk. The code as it was, although not the best looking, is like that for > > a reason. > > This commit is actually the main purpose for this patch set, the previous > commits are just preparing for this commit to look clear. I don't know how that is a reply to my comment...
Okay, I will cherry-pick these commits which you are fine with and push them to the master, as for the rest, I will revise them and submit them later to another bug request. Colsing this.
(In reply to comment #68) > (In reply to comment #67) > > (In reply to comment #28) > > > Review of attachment 264713 [details] [review] [details] [details]: > > > > > > There is a reason why this part does not make use of templates, which is > > > performance. It's much slower (on large diffs) to do the template thing for > > > each hunk. The code as it was, although not the best looking, is like that for > > > a reason. > > > > This commit is actually the main purpose for this patch set, the previous > > commits are just preparing for this commit to look clear. > > I don't know how that is a reply to my comment... Ah, that's not reply, just a little chatty.
Review of attachment 264698 [details] [review]: The 'span.stats' already has 'display: inline-block;' in diff-view.css, so it would would affect nothing, I will commit this.
Review of attachment 264706 [details] [review]: Will commit this with subject "Keep CSS selector naming consistency"
(In reply to comment #48) > Review of attachment 264704 [details] [review]: > > Fine, except for unrelated indent. > > ::: libgitg/resources/diff-view.html > @@ +33,3 @@ > + <!-- ${FILE_STATS} --> > + <span class="file_path"><!-- ${FILE_PATH} --></span> > + </td> > > This is unrelated This is really relavant, the whole change is to make this indentation happen. With previously 'pre-wrap' applied on normal '<td>', the indentation spaces will be treated as spaces in '<pre>' which will break the layout.
Review of attachment 264699 [details] [review]: With a little tweaking, I have this change commited too.
Review of attachment 264714 [details] [review]: This patch depends on the previous one, so it can not be commited now.
(In reply to comment #73) > (In reply to comment #48) > > Review of attachment 264704 [details] [review] [details]: > > > > Fine, except for unrelated indent. > > > > ::: libgitg/resources/diff-view.html > > @@ +33,3 @@ > > + <!-- ${FILE_STATS} --> > > + <span class="file_path"><!-- ${FILE_PATH} --></span> > > + </td> > > > > This is unrelated > > This is really relavant, the whole change is to make this indentation happen. > With previously 'pre-wrap' applied on normal '<td>', the indentation spaces > will be treated as spaces in '<pre>' which will break the layout. Please explain this in the commit message. Otherwise good to commit.
(In reply to comment #76) > (In reply to comment #73) > > (In reply to comment #48) > > > Review of attachment 264704 [details] [review] [details] [details]: > > > > > > Fine, except for unrelated indent. > > > > > > ::: libgitg/resources/diff-view.html > > > @@ +33,3 @@ > > > + <!-- ${FILE_STATS} --> > > > + <span class="file_path"><!-- ${FILE_PATH} --></span> > > > + </td> > > > > > > This is unrelated > > > > This is really relavant, the whole change is to make this indentation happen. > > With previously 'pre-wrap' applied on normal '<td>', the indentation spaces > > will be treated as spaces in '<pre>' which will break the layout. > > Please explain this in the commit message. Otherwise good to commit. It is not clear explained as here, but it has been mentioned in the commit that already pushed.
Is something here still relevant? Should we close this? If there is still stuff, it's pretty crufty right now, with some patches applied, some not, etc. Maybe we can open another bug just to start clean?
I feel sad to see all this work go to waste, but there is little point in keeping this open unless someone wants to work on it.