ChartView: update code for list-rendering

[chel-seyy]

During list rendering, `v-for` is used with `v-if` which can increase
the file processing time, due to additional checks.

Let's follow the vue style guide[1] by replacing these instances with
computed properties, where necessary. Data is only updated when
changed, instead of being generated every time.

[1]: Vue style guide for avoiding use of v-if with for
https://vuejs.org/v2/style-guide/#Avoid-v-if-with-v-for-essential

[chel-seyy]

@reposense/devs Ready for review

[eugenepeh]

Just a hypothesis, let me know if its not possible:

Currently, it seems that scripting takes a long time (hiding java file from Eugene in reposense_RepoSense_master)

Whereas collapsing and expanding all files are less computational intensive

Of course, given the amount of processing difference, there's definitely a difference.
However my point is, whenever the selectedFiles or files in this PR changes, it will trigger the code view to be reload which will create a lot of scripting tasks. So my question is, can we bypass the reload, using less processing power to produce same result by doing something similar to collapsing and expanding the files (just hiding the file content and header from the code view)?

[chel-seyy]

Tried implementing your suggestion by hiding the files using v-show which uses CSS-based toggling so data will remain in the DOM. However, it seems to lag more. :/

After some searching, storing selectedFiles inside computed properties seems to be a better option. selectedFiles will be cached and only recomputed when user selects/deselects checkboxes. This can improve the efficiency of the filtering files process.

Also, Vuejs discourages v-for and v-if used together (current implementation), as this is more inefficient and alike to mapping that iterates through every file for checking.

https://vuejs.org/v2/style-guide/#Avoid-v-if-with-v-for-essential

[eugenepeh]

I see, great work for trying out alternatives.

Also, Vuejs discourages v-for and v-if used together (current implementation), as this is more inefficient and alike to mapping that iterates through every file for checking.

In that case, could multiple vue instances be the causation of the crash in #451?

[chel-seyy]

In that case, could multiple vue instances be the causation of the crash in #451?

There are certainly some examples of v-if used with v-for, but these are inside the v-summary instance:

RepoSense/frontend/src/index.jade

Line 84 in c0f5bfd

.summary-charts(v-for="repo of filtered", v-if="repo.length>0")

RepoSense/frontend/src/index.jade

Lines 107 to 108 in c0f5bfd

	v-for="(slice, j) in user.commits",
	v-if="slice.insertions>0",

Hence, they are not directly related to the crashing of the download feature. Perhaps I can open an issue about this, to improve vue's efficiency?

---

On my side, I have changed the files rendering to use computed properties. However, I noticed that using computed properties does not speed up the selection process significantly. In fact, if v-for is used with v-if (current implementation), the processing is actually faster.

I can push these latest changes up for you to compare the speed difference.

[eugenepeh]

Yes, it seems to be slower after each iteration and eventually crashed on the 4th.

[yong24s]

Can try to move the following code-block out of updated()?

The crash seems to be caused by vuejs going into an endless update.

RepoSense/frontend/src/static/js/v_authorship.js

Lines 211 to 213 in ee21b0d

	document.querySelectorAll('pre.hljs code').forEach((ele) => {
	window.hljs.highlightBlock(ele);
	});

Call it only when code view is open for a new author?

It seems that the above code-block also tries to highlight code of other users as well 🤔.

[yong24s]

I have removed highlightjs and its really fast now.
Test site: https://yong24s.github.io/RepoSense

[yamidark]

I have removed highlightjs and its really fast now.

Nice, tested and barely feel any lag right now 👍.

[yong24s]

Nice, tested and barely feel any lag right now 👍.

But I haven't find out which variable to watch to invoke the hightlightjs 😅

[yong24s]

If you toggle code views of different contribution, highlightjs will render after the first.

Even updated() has been removed https://github.com/yong24s/RepoSense/blob/gh-pages/static/js/v_authorship.js, probably because the code is in <pre><code> 🤔

[yong24s]

My attempt to fix the lag, with comments: yong24s@141a8e6

Test-site: https://yong24s.github.io/RepoSense

Need help to make hljs rendering consistent. Now you may need to click twice to get it to work. 😅

[eugenepeh]

I have removed highlightjs and its really fast now.

highlightjs is meant to give context to the code... without that it is much harder to make sense out of jumbled codes.

[yong24s]

highlightjs is meant to give context to the code... without that it is much harder to make sense out of jumbled codes.

I meant to say I moved hljs out of the Vue#updated. 😓

[chel-seyy]

@yong24s By "hljs rendering" problems, do you mean the toggling of hidden (or non-authored) code?
Because from the test-site preview, the highlighting seemed to work, except for the toggling.

[yong24s]

@yong24s By "hljs rendering" problems, do you mean the toggling of hidden (or non-authored) code?
Because from the test-site preview, the highlighting seemed to work, except for the toggling.

It don't always work in the code view. I'm not sure where to best put the hljs code 😅.

[ongspxm]

It don't always work in the code view. I'm not sure where to best put the hljs code 😅

one other way is to wrap the whole code thing into a component of its own and put the hljs inside the component, so one call will just be in charge of one "file"

[yong24s]

@chelseyong

You want to work on the hljs optimization? I can create a new issue and assign it to you.

Let this PR be code view optimization only.

[chel-seyy]

@yong24s Okay can

[yamidark]

Don't really see a different a response time when using the file type filter though.
Currently, hiding of the file types is very fast (almost instantaneous), but loading of the file types may take up some time (depending on the size).

From testing of Eugene in reposense_RepoSense_master (loading of file types):

Current master:

This PR:

Don't see much of a time difference between the two (and this PR actually may be slower!), and both still takes up a long time for scripting.
Can try to look into how we can speed up the loading time of file types?

[yamidark]

Changes requested as above

[chel-seyy]

@yamidark Okay, do you mean the scripting in the loading of tab?

[yamidark]

@yamidark Okay, do you mean the scripting in the loading of tab?

Nope, I meant the loading of all the files in the code panel, when after we hide all the files types and re-show them (uncheck and check the All box), which is supposed to be what this PR is working on right (according to the title)?

[chel-seyy]

Noted, the message was not clear before.

[yamidark]

@chelseyong Well if you could improve the time for initial loading of all the codeview files as well, that will be good too 😄

[eugenepeh]

hmm now that speed isn't much of an issue, perhaps focus on cleaning up?

[chel-seyy]

@eugenepeh Okay. May I know what do you refer to as cleaning up?

[eugenepeh]

@eugenepeh Okay. May I know what do you refer to as cleaning up?

just pretty much whatever you're doing now, following suggestion given in https://vuejs.org/v2/style-guide etc.

Let us know when you're done.

[chel-seyy]

@eugenepeh PR is ready

[yamidark]

@chelseyong Remember to update the PR title and message since this PR objective change to that of cleaning up code

[yong24s]

Nice work by following best practices. 👍

[yamidark]

Slight nits to proposed message:

During list rendering, `v-for` is used with `v-if` which can increase
the file processing time, due to additional checks.

Let's follow the vue style guide[1] by replacing these instances with
computed properties, where necessary. Data is only updated when
changed, instead of being generated every time.

[1]: Vue style guide for avoiding use of v-if with for
https://vuejs.org/v2/style-guide/#Avoid-v-if-with-v-for-essential

@chelseyong Do add 'footnotes' in your message for when you want to provide additional info through a link

[chel-seyy]

@yamidark okay noted