ENH Feature metadata coloring command pattern #440

gwarmstrong · 2020-10-31T00:13:32Z

@kwcantrell and I put together a proposed refactoring of feature metadata coloring using the Command Pattern. I am tagging this as progress on #439 , but there are probably also others that could apply. We were hoping to get some feedback on the general design, and if the reception is positive, would be happy to flesh out more of the docs/tests.

But the general mechanism we introduce here is the Command pattern.

Namely, the proposal would be that updates to Empress plots should be routed through Commands, where you encapsulate all of the information needed to perform an update in the create of the Command object. This can seen, for example in the lines:

            let command = new Commands.ColorFeatureTreeCommand({
                empress: scope.empress,
                collapseClades: collapseChecked,
                lineWidth: lw,
                colBy: colBy,
                col: col,
                coloringMethod: coloringMethod,
                reverse: reverse,
            });

where all of the information needed to color the tree based on it's feature metadata is passed to the constructor ColorFeatureTreeCommand.

This command can then later be executed by calling the execute method, i.e, command.execute();, which actually performs the tree coloring based on the code in ColorFeatureTreeCommand's execute method.

We have also included a CommandManager class which maintains a queue of commands to execute, and a history of commands that have been executed via the CommandManager. It also comes with utility functions for helping serialize and de-serialize the history. If all of the UI elements are routed through the command manager, this could be used to completely reconstruct the state of the empress plot, enabling save/load/playback feature (that admittedly could get memory intensive for long running sessions, given the current implementation).

Using the command pattern throughout this project could also be useful for a variety of other 'wishlist' items, such as programming custom animations, i.e.,

command1 = new Commands.ChangeTreeLayoutCommand(...);
command2 = new Commands.ColorTreeByFeatureCommand(...);
command3 = new Commands.AddBarplootCommand(...);

commands = [command1, command2, command3];
commands.forEach(function(command) {
    setTimeout(function() {
        command.execute();
    }, 5000);
});

And you could have some event bus subscriber that creates commands. E.g.,

function Subscriber(bus, ...) {
    ...
}

Subscriber.prototype.handle = function (event) {
    ...
    colorScheme = // something you are to glean from `event`
    let command = new Commands.ColorFeatureTreeCommand(..., colorScheme, ...);
    command.execute();
    ...
}

Which should hopefully allow for relatively extensible interaction code (e.g., interacting with Emperor)

Co-authored-by: kwcantrell

dhakim87 · 2020-10-31T00:22:12Z

        let command = new Commands.ColorFeatureTreeCommand({
            empress: scope.empress,
            collapseClades: collapseChecked,
            lineWidth: lw,
            colBy: colBy,
            col: col,
            coloringMethod: coloringMethod,
            reverse: reverse,
        });

I'm not sure I totally follow the example. Is there an interface that commands must follow - Some set of fields they all share?

Why is it empress: scope.empress rather than scope: scope.empress?
Should colorFeatureTree be split into colorFeatureTree and lineWidthFeatureTree?

It looks like colBy, col, and coloringMethod are all optional? Color by (dictionary?), Color (fixed color?), Coloring Method (function?) - coloringMethod would have to be the name of a function I guess, in order to allow json serialization. Or is this intended to be an enum specifying whether colorBy or color should be used?

I think you might want a command_type: xxx added to all the commands to make parsing easy.

emperor-helper · 2020-10-31T00:24:37Z

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv

dhakim87

I really like this. I think this is the right skeleton for a message passing solution. I have minor comments on removing boilerplate and optimizations that may eventually become issues.

dhakim87 · 2020-10-31T00:24:39Z

empress/support_files/js/commands.js

+
+    function CommandManager(empress) {
+        this.empress = empress;
+        this.executed = [];


This queue is a little weird. If the design involves some external message bus, I would think that message bus would keep track of all commands that pass through it, or even some specific logger component. I don't see a value in having every instance of empress maintain commands that it executed.

One use case this one makes sense for is Undo/Redo functionality, but I don't think that really works with a message bus architecture and I haven't heard it being requested. (It also requires all commands to store enough state to be invertible)

Right so this CommandManager is intended just to queue up commands for empress only, and hopefully be able to replay some actions on empress. We would require a separate class to manage interactions with an event bus.

dhakim87 · 2020-10-31T00:30:19Z

empress/support_files/js/commands.js

+        var scope = this;
+        object.forEach(function (commandObject) {
+            var commandConstructor = scope.commands[commandObject.command];
+            var args = commandObject.arguments;


I feel like this could be done with a JSON.parse(), I don't quite get the value of looking up the constructor and passing it arguments yet.

Ahhh! Commands need to know their type for the execute calls!

I'm not sure how explicitly you want to type the command objects. You might be able to have javascript handle all the serialization and deserialization without any per Command boilerplate code. It could certainly be done with some wonky prototype trickery, or by having each command hold a separate untyped CommandArgs argument. But if you want it to be as explicitly typed/type safe as possible, the current mechanism with copies is as close as you can get in javascript.

dhakim87 · 2020-10-31T00:32:36Z

empress/support_files/js/commands.js

+    };
+
+    CommandManager.prototype.executeNext = function () {
+        let command = this.toExecute.shift();


This should use a linked list or an array backed queue one day before allowing replaying events from a log, otherwise we'll get burned by quadratic cost of shifting items out of the array list.

Hmmmm, any pointers to places where people have written an actual linked list/dequeue in javascript? Most of the implementations I have found just use shift 😬

dhakim87 · 2020-10-31T00:34:03Z

empress/support_files/js/commands.js

+        }
+    };
+
+    CommandManager.prototype.getSerializable = function () {


Useful for save/load log but I think this functionality should be moved to a separate component and events forked to both the empress command manager and the logger command manager or something.

dhakim87 · 2020-10-31T00:37:19Z

empress/support_files/js/commands.js

+        this.reverse = arguments.reverse;
+    }
+
+    ColorFeatureTreeCommand.prototype.toObject = function () {


Can probably wrap this into a Command base class - just need to loop over public fields/properties

Try Object.getOwnPropertyNames, or anything nearby on this page: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyNames

dhakim87 · 2020-10-31T00:40:47Z

empress/support_files/js/commands.js

+    };
+
+    function ColorFeatureTreeCommand(arguments) {
+        this.empress = arguments.empress;


It will be up to empress to actually apply this command at some point. Do you want the Command instances themselves to understand how they are applied, or do you want the empress code to parse the command and apply them?

If the first, constructing the actual command instances has some value. If not... why shouldn't empress just look at the .command field of some arbitrary json object?

Oh... looks like its the first 👍

dhakim87 · 2020-10-31T00:42:10Z

empress/support_files/js/commands.js

+
+    CommandManager.prototype.executeNext = function () {
+        let command = this.toExecute.shift();
+        command.execute();


Where does execute() live? How does it avoid taking an empress object?

Ahhh, sneaks into the Command object when it gets parsed. I think it might be cleaner to pass it into the execute call rather than parsing - doing so would allow you to use json serialization and deserialization for what otherwise becomes a ton of boilerplate creation of constructors and to_object.

^ended up following this and switching to execute(empress) and it is a lot cleaner.

dhakim87 · 2020-10-31T00:59:30Z

empress/support_files/js/commands.js

+    };
+
+    ColorFeatureTreeCommand.prototype.execute = function () {
+        this.empress.resetTree();


What are the odds that resetTree is interminably slow? Do we need this to be a separate command? Do we need FreezeLayout ResumeLayout commands like exist in UI frameworks to allow for numerous bulk operations?

fedarko

This looks great, thanks so much! I have some comments -- they're mostly small-scale things, though. By and large I think the overall structure looks great. Having this in will clean things up a lot :)

empress/support_files/js/side-panel-handler.js

fedarko · 2020-11-02T23:34:22Z

empress/support_files/js/commands.js

+        });
+    };
+
+    function ColorFeatureTreeCommand(arguments) {


It seems to me like the individual command classes (which as @dhakim87 suggested would be derived from a Command base class) should have their own concrete requirements for arguments, although I guess the base class wouldn't have any such requirements.

Hm, I'm not sure what the best way to set this up would be. Knowing JS, there are probably like a billion ways to do this, and you've probably already thought of some of these :) To add to the noise, one idea that should work would be (assuming a Command base class will be eventually added) adding a method to that base class that goes through the stuff in arguments and checks it against, say, a Command-subclass-specific array of needed arguments, and raises an error if some needed stuff isn't specified (using some of the property stuff Dan suggested -- and/or we can probably use some of underscore.js to do this easily, e.g. _.keys() and _.intersection()).

fedarko · 2020-11-02T23:42:12Z

empress/support_files/js/commands.js

+        return objects;
+    };
+
+    CommandManager.prototype.parse = function (object) {


One of those "not a huge deal but" things -- I think we should try to avoid object as a variable name, just because Object is a keyword in JS.

I'm also a bit confused by how this function works, since it seems to me on first reading that the object parameter is actually an Array of Command objects (since forEach() isn't a property of Objects in JS). If this is the case, I would suggest renaming object to something like commands (I know docs aren't a big focus of the initial work here but this would be good to make clear).

Make sense and updated now!

fedarko

I haven't finished going through all of the new functionality yet (will do that later this week), but here's what I have by now.

fedarko · 2020-11-11T02:04:59Z

empress/support_files/js/commands.js

@@ -0,0 +1,285 @@
+define(["underscore"], function (_) {
+    function CommandManager(empress) {


Not all that urgent, but it would be good to add a comment here similar to what the Command class has below (just to make it clear that this is also a "class", at least kinda). There are examples of this (for non-explicitly-declared JS classes in Empress) in bp-tree.js and empress.js.

Added in 00baae9

fedarko · 2020-11-11T02:06:11Z

empress/support_files/js/commands.js

+        this.toExecute = [];
+    }
+
+    CommandManager.prototype.push = function (command) {


I guess now that we're at the stage of getting ready to merge this in, it'd be nice to add some basic docs for these functions (sorry, I realize this is going to be like 4 separate comments ._.)

At the very least, it'd be good to be very clear that a first-in first-out queue is used (... at least that is what I think is done from reading this).

Added in 00baae9

empress/support_files/js/commands.js

Co-authored-by: Marcus Fedarko <[email protected]>

gwarmstrong added 2 commits October 30, 2020 16:10

ENH add command pattern for color feature tree

f41c7ab

Co-authored-by: kwcantrell

STY make commands jsstyle

a6ca629

Co-authored-by: kwcantrell

gwarmstrong requested review from ElDeveloper and fedarko October 31, 2020 00:13

dhakim87 reviewed Oct 31, 2020

View reviewed changes

fedarko reviewed Nov 2, 2020

View reviewed changes

gwarmstrong added 4 commits November 5, 2020 17:02

Apply suggestions from code review and add more docstring

eed0070

DOC fix documentation and exports

3c36751

TST test commands

4624b4d

STY command tests

3c96105

gwarmstrong changed the title ~~WIP: ENH Feature metadata coloring command pattern~~ ENH Feature metadata coloring command pattern Nov 10, 2020

fedarko self-requested a review November 11, 2020 01:34

fedarko reviewed Nov 11, 2020

View reviewed changes

gwarmstrong and others added 2 commits November 16, 2020 10:34

DOC add comments for CommandManager

00baae9

FIX composite command comment

898278f

Co-authored-by: Marcus Fedarko <[email protected]>

gwarmstrong requested a review from fedarko November 16, 2020 21:00

ElDeveloper closed this Dec 16, 2020

ElDeveloper reopened this Dec 16, 2020

gwarmstrong closed this Aug 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Feature metadata coloring command pattern #440

ENH Feature metadata coloring command pattern #440

gwarmstrong commented Oct 31, 2020

dhakim87 commented Oct 31, 2020

emperor-helper commented Oct 31, 2020

dhakim87 left a comment

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

gwarmstrong Nov 4, 2020

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

gwarmstrong Nov 5, 2020 •

edited

Loading

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

dhakim87 Oct 31, 2020

gwarmstrong Nov 10, 2020

dhakim87 Oct 31, 2020

fedarko left a comment

fedarko Nov 2, 2020

fedarko Nov 2, 2020

gwarmstrong Nov 10, 2020

fedarko left a comment

fedarko Nov 11, 2020

gwarmstrong Nov 16, 2020

fedarko Nov 11, 2020

gwarmstrong Nov 16, 2020

		@@ -0,0 +1,285 @@
		define(["underscore"], function (_) {
		function CommandManager(empress) {

ENH Feature metadata coloring command pattern #440

ENH Feature metadata coloring command pattern #440

Conversation

gwarmstrong commented Oct 31, 2020

dhakim87 commented Oct 31, 2020

emperor-helper commented Oct 31, 2020

dhakim87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwarmstrong Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedarko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedarko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwarmstrong Nov 5, 2020 •

edited

Loading