Implement #61 (Object Insertion Point)

stardust66 commented 7 years ago

Feature suggested in #61. I modified MathObject and everything that inherits from it to also take an optional folderIdx argument in the constructor, which is the index of the folder in math3d.mathTree. I also added to the folder template, treeCtrl, and addObjectCtrl, so that clicking on a folder highlights it and any new object you add will go to that folder. I'm working on making users able to insert after any object. It should be a similar process.

stardust66 commented 7 years ago

Here are some commits for inserting right after a certain item in the tree. The main problem is that when the selected item gets reordered, it's no longer selected and the item that takes its place gets selected instead. A solution would be to make MathObjects have a selected property so it remembers whether it's selected or not when it's reordered. Do we want that?

ChristopherChudzicki commented 7 years ago

Cool! The insertion point has always bugged me, and this looks like good steps toward resolving that.

I can give the code a thorough look-through tonight (after 9pm EST).

Added a selected property sounds like a reasonable idea, but I would add that property to mathObject.settings in order to facilitate serialization.

One other comment: As much as possible, I've tried to keep math3d.js agnostic about the UI, and take care of all UI issues inside the templates and math3d_app.js. This was a very useful separation especially in the beginning when I hadn't settled on a particular design for the UI. But I think it is good to try and continue with this principle.

ChristopherChudzicki commented 7 years ago

@stardust66 Ok, I've been looking at your code a bit more. Two comments:

MathObject Constructor Argument Names

In the spirit of "code is read more often than it is written", I think descriptive variable names are really important. With that in mind, I'd like to suggest something more descriptive than folderIdx and eleIdx in the MathObject constructors. (eleIdx also made me initially think this parameter was the id of an HTML element. That might coincidentally be true---I'm not sure---but the argument in this context has a more abstract meaning relating to the insertion point into the list mathTree[j].objects).

How about something like constructor(math3d, settings, insertionPoint)? Where insertionPoint has form

insertionPoint = {
    folderIdx = <value>,
    afterPosition = <value>,
}

Avoiding `$rootScope`

It looks like you're using $rootScope in treeCtrl and addObjectCtrl to pass information (about the selected MathObject) between these controllers. I'm still a beginning with angular, but it seems like using $rootScope should be avoided.

Earlier, you said:

A solution would be to make MathObjects have a selected property so it remembers whether it's selected or not when it's reordered.

This approach could help eliminate your use of $rootScope. But I would suggest a slightly different approach:

in treeCtrl, attach the selected MathObject itself (not the indexes) to math3d as math3d.selected.
add a method to MathObject called getMathtreePosition (or something similar) that returns {folderIdx:val, positionWithin:val} (or something similar).
inside addObjectCtrl, use math3d.selected. getMathtreePosition() to get the current position of the selected object.

If you store the selection status of each individual object (mathObject.selected = true/false) then you'll have to turn off the old object when you select a new one. I think the above approach avoids this hassle.

CSS Question

Is .item-slected a CSS class added by angular-ui-tree? I didn't see you adding it anywhere.

stardust66 commented 7 years ago

@ChristopherChudzicki Thank you for the comments. I'll be working on this. The CSS class is added with a ng-class directive here, on line 33.

How do you think getMathtreePosition should be implemented? It needs to work when the object gets reordered, and the most obvious way seems to be iterating overmath3d.mathTree and returning the position of the object, but that seems computationally expensive.

ChristopherChudzicki commented 7 years ago

@stardust66 Oh, I see. I don't know how I missed that ng-class. Regarding

How do you think getMathtreePosition should be implemented? It needs to work when the object gets reordered, and the most obvious way seems to be iterating over math3d.mathTree and returning the position of the object, but that seems computationally expensive.

I think what you outline sounds fine. Some thoughts:

Most importantly: I don't think you need to worry about optimizing performance in this case. How often is this function getting called? Only when an object is created, so once every several seconds? That's a pretty long timescale in javascript.
A traditional javascript for loop is very fast. According to this blog post Performance of for loops with Javascript a tradition nested for loop with 10,000 iterations takes a couple ms to evaluate. (Though that's clearly hardware dependent). So pretty fast.

So, if you had a for loop that is either (1) of truly enormous length or (2) called frequently (for example, every frame) then I would worry more. But I don't think you need to worry here.

stardust66 commented 7 years ago

In the new version, the selected object is attached to math3d.selected and the insertion works pretty well. The selection status stays with the object after reordering too. I tried attaching the object to math3d.settings.selected but that affected the saving. I got the following error.

I tried adding some log statements in the deepCopyValuesOnly function, and found out that it seemed to be copying the keys style, classList, and dataset repeatedly.

What do you think is happening here? Here's the problematic commit:

ChristopherChudzicki commented 7 years ago

Oh. I think what's happening with that call stack error is that by attaching a mathObject to math3d.settings, math3d.settings has circular references. (math3d.settings.active.math3d = math3d). This is not a bad thing in general: circular references are useful. But it is a bad thing here because objects with circular references cannot be serialized.

(deepCopyValuesOnly was trying to serialize math3d.Settings. By the way, it was the deepCopy part that is problematic, not the ValuesOnly part.)

Now: do you think active object status should be stored when a user saves the graph? It would be nice, but not crucial.

If you want to store the information, then you'll need to serialize it, and it should go in either math3d.settings or mathObject.settings. (But the data needs to be something that can be serialized, like strings or simple objects.)

If you want to store the active object information upon graph save, then I think your original approach—storing active status as a simple true/false on mathObject.settings—will facilitate serialization. The only difficulty I foresee is what I mentioned before: whenever you mark one object active, you'll need to mark the previously active object as inactive.

You decide which way you want to go with this :).

PS: By the way, here's the deal with Math3d.settings and MathObject.settings:

Both of these objects contain several getters and setters. So whenever you do something like mathObject.settings.visible = false, the setter for visible automatically runs the mathObject.setVisible() method to toggle object visibility. Take a look at some of the setters and getters if you haven't.
math3d.settings and mathObject.settings are also the things that get serialized when you save a graph. The setter and getter functions can't be serialized themselves, hence the use of deepCopyValuesOnly.

stardust66 commented 7 years ago

I think the saving part is convenient but there's a good reason not to have it. When people save the graph they probably don't expect the selection to be saved because it intuitively seems like it's tied to the session, not the graph itself. So people might save a graph leaving an item selected, not realizing that that gets saved, and the next person opens the graph finding an arbitrary item highlighted. So I think I'm just going to revert the last commit if that's okay with you @ChristopherChudzicki.

Thanks for telling me about the getters and setters. I need to spend some time reading math3d.js. It'll help me with javascript. Also, you might want to change the style for the selected item before merging. I just picked a random one.

ChristopherChudzicki commented 7 years ago

So people might save a graph leaving an item selected, not realizing that that gets saved, and the next person opens the graph finding an arbitrary item highlighted.

I think you are right. Better not to save the active object.

stardust66 commented 7 years ago

TODO

[ ] Make graph loading order consistent with its placement in the list when adding in between two items.

ChristopherChudzicki commented 6 years ago

@stardust66 Despite the graph ordering issue we discussed before, I want to merge this. I think having the insertion point correct would be great. I'm sorry it's taken me so long, Jason.

That said... I made some PRs earlier tonight that caused merge conflicts. I am pretty sure the merge conflicts are just whitespace that got inserted by Atom, but I can't push to your branch so I can't resolve. Do you feel like resolving? If so, I will test a little bit more then merge, hopefully before my talk on Saturday. (I live dangerously.)

stardust66 commented 6 years ago

I thought I just resolved the conflicts, but the warning didn't seem to go away? I think you have to do something @ChristopherChudzicki.

ChristopherChudzicki commented 6 years ago

@stardust66 You're right, conflicts resolved.

stardust66 commented 6 years ago

You might also want to change the style of the selected item. What do you think would be good? Or you can merge this and do it yourself, which would be easier.

ChristopherChudzicki commented 6 years ago

@stardust66 Merged and pushed to Heroku. Thanks for this, Jason!

ChristopherChudzicki / math3d