Consolidate state management into a single controller for user selected options

[kdahlquist]

Right now each feature's state is held in different variables, chosen by the person who wrote that feature. This needs to be consolidated into a single controller because GRNsight is not remembering the state. They need to be synchronized.

[dondi]

@yshin4 will focus on this while @eileenchoe finishes up #577, then will merge and pair for the menu/layout work.

[kdahlquist]

Based on what I see in current beta, we really need this.

[yshin4]

Noticed that when the slider value is changed and the graph reloads, the graph itself is reloaded with default values (link distance 500 and charge -50). But the sliders do not reload and they stay the same as before the reload. The values stay as how the user changed, but it is not applied to the graph so there is an inconsistency between the slider value and the graph.

Also, the graph froze and did not react to the change according to the sliders when I was trying to change the value for linked distance, similar to how grid layout sometimes does not toggle properly. I do not know how to recreate this event yet.

[eileenchoe]

Completed state management for node and edge features while working on #462

[yshin4]

I don't think I can reorganize all the parts I'm working on for the menu into one controller. Focusing on getting the new menu working right now and I probably can't refactor all of them in slider/upload.

[yshin4]

@eileenchoe's updates are live on beta 2.3.7 now.

[kdahlquist]

I think this was included in v3.0.0 release. @dondi should do the "review requested" this one and close.

[mihirsamdarshi]

@dondi, my latest commits have the basis of the architecture down, however I have a technical question to ask you. In line 19, I have to re-call drawGraph in order to implement the gray edges. How would I import the other parameters that are implemented? Would that all go in grnState as well?

[mihirsamdarshi]

@dondi now that a framework was established, I am going to start moving the gray edges into the single state controller

[mihirsamdarshi]

additionally @dondi what should I be looking for to consolidate with annotateLinks in grnstate.js?

[kdahlquist]

Question for @dondi: Are we going to release the beta-consolidation stuff with the 3.1.0 release tomorrow? @mihirsamdarshi is thinking that he doesn't want to do that until it's all finished.

I thought that as long as nothing is broken, we should, but he is worried that since it hasn't been thoroughly tested, something might be broken.

Please comment.

[dondi]

@kdahlquist @mihirsamdarshi I see nothing wrong with releasing this with 3.1.0. True that we haven't done UI-level unit tests, but there are some model-level unit tests and the code review cycle on this set of changes was pretty strict. Unless there is a blatant regression, I am OK with releasing.

The overall task is large enough that waiting for it to be all finished will hinder more than help. Better to dole this out piecemeal and let it "settle" than to keep it on the side for too long.

[mihirsamdarshi]

So far, as of release v3.1.0, the following features have been consolidated into out MVC framework: network object, normalization, edge weight display options, enabling/disabling edge coloring/thicknesses, gray edge threshold, and displaying gray edges as dashed lines. Going forward, the whole of upload.js can be added to the MVC, as well as figuring out a way to move the whole of graph.js. Moving node-coloring.js and sliders.js should be next on our priority list.

[mihirsamdarshi]

I am currently working on sliders.js, moving over the constants right now

[mihirsamdarshi]

@dondi, I believe the next best course of action is to move updateSliderDisplayedValue to update-app.js, what do you think?

[dondi]

OK @mihirsamdarshi give that a shot. Looks slightly trickier than it looks, because I think that code applies to multiple sliders. Each slider becomes its own property in the model, but ideally you still find a way to use the same view and controller code, with the property being stored as some kind of variable or parameter.

[mihirsamdarshi]

I see that now, and I believe the following code does that. However, I can't seem to decipher what it is actually doing. This is on line 20 in update-app.js

var SLIDER_ADJUSTER = {
    charge: function (sliderController, value) {
        sliderController.simulation.force("charge").strength(value);
        sliderController.simulation.alpha(1);
    },
    link: function (sliderController, value) {
        sliderController.simulation.force("link").distance(value);
        sliderController.simulation.alpha(1);
    }
};

[dondi]

After looking at the code, @dondi recommended that the sliders.js code can be broken up for now, into the new MVC structure, without having to copy the existing functions. Just code charge and link changes according to the MVC cycle and let’s see how that looks when you’re done.

[mihirsamdarshi]

@dondi I just pushed the first part of my split, I was wondering if I was on the right track or if I should approach it in another way? I was planning on ridding the previous sliderGroupController of its array properties, to make it easier to split the functions within the controllers. Additionally, I was wondering what this function this.addForce(simulation) was actually doing? My understanding is that it is creating an array from the object SLIDER_ADJUSTER. However I was wondering, are the contents of the object included in that array (i.e. for the charge property in SLIDER_ADJUSTER is the function included?

[dondi]

After code review of @mihirsamdarshi’s work in progress, the green light was given to proceed. After separating the controller code, some functions will need to go to the view vs. controller modules. Also, some code duplication is expected, and a consolidation step is expected at the end. Unit tests are expected as well.

[mihirsamdarshi]

I am currently in progress with the move. Currently, the move has broken the sliders on my branch. I don't know if I should restart, considering our goal is to move sliders with minimal detriment to functionality. I also ran into a question regarding whether we should move sliderObject itself to grnState or its individual components.

[dondi]

@mihirsamdarshi has identified a new approach to refactoring the slider code which will require him to back out some of his work and start over. However he also has some work which can be considered as done. His upcoming pull request sometime Monday will include this work, but with the slider code reverted so that he can go on a fresh start after this cycle.

[dondi]

@mihirsamdarshi is back in the saddle and will aim to finish the slider code refactor over the weekend. The goal is to have a pull request by early next week in order to have a beta ready for @kdahlquist on Thursday.

[kdahlquist]

@mihirsamdarshi reported progress on the sliders and says we are on track to complete the refactoring of all features by the end of the semester. He is going to be out of town on the weekend and has a couple of midterms next week, so his next PR will likely not come until next Wednesday night.

[dondi]

@mihirsamdarshi is continuing to refactor the sliders code and is getting close. Emphasis was made on enforcing the one-directional flow of MVC information even if some iterations are carrying the same values.

Pull request is anticipated for the weekend.

[kdahlquist]

@mihirsamdarshi has questions for @dondi in PR #706 .

Here is the list of what has been completed so far (@mihirsamdarshi, please edit this comment if I got something wrong):

• Lock/unlock Force Graph Parameters
• Reset Force Graph Parameters
• Hide/show edge weights
• Edge weight normalization factor set/reset
• Gray threshold slider
• Show gray edges as dashed
• Link distance and charge sliders are almost done

What hasn't been done:

• Grid layout
• Node coloring
• Zoom
• D-pad
• Viewport size/restrict graph to viewport.

The only thing in the menu that is not on the left-hand panel, is format edges based on weight values (i.e., turn on/off edge coloring). Is that one done or not?

To finish off the semester, let's get link distance and charge sliders done.

[dondi]

Carrying on thread from PR #706 @mihirsamdarshi will refactor the "layout" property of the app to the new MVC structure and aim to make the PR mergeable into beta after that is done.

[mihirsamdarshi]

Just as I guessed, moving the code that specifies layout to grnState fixed any issues that we were having

[mihirsamdarshi]

@dondi the code has all been compartmentalized, and so far everything except for the code that controls the DOM of the graph is working. For that I have created two new sets of functions within graph.js setNodesToGrid() and setNodesToForceGraph() at around line 1234. I know that I am missing something extremely basic, because this code can't be called from within updateApp because there is a TypeError stating that drawGraph.setNodesToGrid() or drawGraph.setNodesToForceGraph() are not functions.

[kdahlquist]

In its current form those functions are declared as local variables and this is why they are not visible to the outside. drawGraph should probably transition to an object eventually, so that its functions can be viewed as methods on the graph. As a transitional step, try to assign those functions to this. This should make those functions visible to outside code as properties of the drawGraph function itself.

[mihirsamdarshi]

All major features, except node coloring, have been added to the new architecture structure. Discussions are underway for the conversion of graph.js to be an object.

[kdahlquist]

Version on beta looks good. Additional refactors will be done as new issues.