Updated UI for workflows

[harshit-soora]

Workflows Epic Link - #4338
UI Discussion and feature demo - #4625

Just UI changes, no stitching with the backend.

Todo tasks required:

1. Save the edge and launcher position in backend, save the dependency graph and allow submitting workflow. Add workflow submit buttons.
2. Visually show whether or not a launcher was successful(green), failure(red) or didn't get launcher (greyed-out) due to pre-requisite unsatisfied
3. Allow mechanism to have more than one instance of launchers, backend code change needed to support.
4. Edges can still overlap ex. 1->2, 2->3, 1->3 in linear fashion. Maybe we detect this and add some vertical space between to prevent?

This is how new UI looks like:

[Bubballoo3]

Since our javascript lives outside of ERB (once it's in its own file), this will have to be reworked to get the project.id from the html somewhere.

[Bubballoo3]

I think there is a way to do this better with jbuilder, so that you can generate small html blocks from the backend. Jquery also provides some better ways of making requests than fetch. Check out this method from active_jobs.js that does something analogous when extended details are requested.

ondemand/apps/dashboard/app/javascript/active_jobs.js

Lines 65 to 74 in 5ecfabc

	$.getJSON(jobDataUrl, function (data) {
	// Open this row
	row.child(data.html_ganglia_graphs_table).show();
	// Add the data panel to the view
	$(`div[data-jobid="${escapeHtml(row.data().pbsid)}"]`)
	.hide()
	.html(data.html_extended_panel)
	.fadeIn(250);
	// Update the status label in the parent row
	tr.find(".status-label").html(data.status);

[Bubballoo3]

I like these changes, the grid structure makes it seem much more organized from the start and the overlap system stops most ambiguity.

On the functionality side, there are a few things I still notice

1. You cannot have more than one instance of a launcher. This was something we touched on in our meeting and was possible under the current approach to running workflows. In addition to enabling reuse and saving time for users, this would make the UI much easier to test
2. The grid doesn't fit neatly in the canvas panel. On my screen at full width I can fit 5.5 grid columns as you have defined them. Adjusting that it looks like the grid stays fixed as the screen resizes, so you not only can have launchers partially cut off despite being in valid grid locations, but if you adjust too much (say by moving from a fullscreen to a windowed view), you can 'lose' launchers without noticing. The easiest fix for this IMO would be a resize handler that adjusts the canvas size and keeps the grid locations balanced and visible. Obviously with the scrolling there is only so much we can do, but I think that would help a lot.
3. The grid size could be expandable. This prevents an implicit size limit imposed by the grid. I think expanding should only happen in one direction just for uniformity (every graph is 12 potential nodes wide, but unbounded in length, for example). All good if this is in your plans and hasn't made it in yet.
4. Edges can still overlap, which is the final spot where ambiguity may creep in. If I have nodes 1,2,3 horizontally in a row and I have edge 13 and edge 23, edge 23 is completely obscured. Maybe we detect this and add some vertical space between to prevent? The same issue could happen regardless of the axis they are arranged upon but the idea is making any overlapping lines into parallel lines that still hit the launcher's correctly.

On the implementation side there are still some points about the javascript factoring and organization that I don't think you've gotten to yet (totally fine) but one thing to keep in mind with that is limiting the prevalence of pixel computation in lots of different functions. The use of mathematical terms and patterns is great and I can generally tell what a block of computations is doing at a glance, but this is not generally the case for everyone and we should hide all of that nitty gritty stuff behind small descriptive functions to make it more readable and intuitive. It would also be nice if we could avoid some of the pixel handling to css transforms and structures but that will be easiest to do once the computations are factored into the most fundamental blocks.

I do think these changes do a great job at addressing some of my earlier concerns, it is no longer possible to overlap launchers and there is always enough room to fit an arrow neatly in between nodes, which I think will save us a lot of work later on with rendering. Great work!

[Bubballoo3]

The zoom feature works well, but maybe resize the initial canvas so that min zoom shows the whole thing? For me it still cuts off the last column at 50% and this is not impacted by any browser zoom.

[harshit-soora]

The zoom feature works well, but maybe resize the initial canvas so that min zoom shows the whole thing? For me it still cuts off the last column at 50% and this is not impacted by any browser zoom.

Oh now I understand your comment, the idea is to show complete grid cell not half cut-off; basically either resize the cell height-width or zoom such that it shows complete cells. Got it.

[Bubballoo3]

Yeah absolutely. And the zoom thing does cut you some slack because you don't have to factor screen size in quite as much, if the starting size cuts something off they can just zoom out. But that only gets you around the problem if the fully zoomed out screen shows you the whole thing (ideally no scrollbars at 50% zoom).

[harshit-soora]

I like these changes, the grid structure makes it seem much more organized from the start and the overlap system stops most ambiguity.

I have handled 2, 3. For rest I have added as next todo task in the PR description. Tested the UI code multiple time.

@Bubballoo3 Now open to formatting code review, any feature request will handle in separate PR.
Some code factoring such as snake casing, identical organization of if/while/for is done.

[Bubballoo3]

I didn't mean to suggest you had to use snake case, taking a peek at the rest of the javascript code in the project it looks like it is used sparingly, and it provides nice visual separation if we prefer snake case for ruby and camel case for javascript. I actually think the snake case works well for everything ending in a dimension (x,y,l,w), so cell_w is fine compared to cellW but for regular variables like selected_edge I think camel case is better.

The main formatting changes I had in mind was adding some of the class structure we had discussed. I see basically 4 places that we can use classes to avoid passing around pixel values quite so much, and make it all a little clearer. I am including some example code from chatgpt but that is mostly to show the functionality I see these classes having, and don't know if the implementation is exactly right.

1. Box
   Stores positional information and handles movement itself. Other code can tell it to move to a specific position (when dragging) or tell it to snap into the grid (when dropped).

   class Box {
     constructor(el, row, col, w, h) {
       this.el = el;
       this.row = row;
       this.col = col;
       this.x = 0;
       this.y = 0;
       this.w = w;
       this.h = h;
     }
   
     moveTo(x, y) {
       this.x = x;
       this.y = y;
       this.el.style.left = x + 'px';
       this.el.style.top = y + 'px';
     }
   
     snap(cellToXY) {
       const pos = cellToXY(this.row, this.col);
       this.moveTo(pos.x, pos.y);
     }
   }

2. Edge
   Similarly to the box, we should let the edges handle their own position updates. Since we only create and update lines, this doesn't need much more.

   class Edge {
     constructor(fromBox, toBox, el) {
       this.from = fromBox;
       this.to = toBox;
       this.el = el;
     }
   
     update() {
       const { from, to } = this;
       const cx1 = from.x + from.w / 2, cy1 = from.y + from.h / 2;
       const cx2 = to.x + to.w / 2, cy2 = to.y + to.h / 2;
       const dx = cx2 - cx1, dy = cy2 - cy1;
       const start = intersectRect(cx1, cy1, from.w, from.h, dx, dy);
       const end = intersectRect(cx2, cy2, to.w, to.h, -dx, -dy);
       this.el.setAttribute('x1', start.x);
       this.el.setAttribute('y1', start.y);
       this.el.setAttribute('x2', end.x);
       this.el.setAttribute('y2', end.y);
     }
   }

3. Pointer
   This gives you a single source for getting the mouse position, and ensures we are only updating what we need to. Other code reads from the pointer, and the function pointerInStage(e) becomes clearer as pointer.update(e)

    class Pointer {
      constructor(stage, zoomRef) {
        this.stage = stage;
        this.zoomRef = zoomRef; // pass a reference to the current zoom
        this.x = 0;
        this.y = 0;
      }
    
      update(e) {
        const r = this.stage.getBoundingClientRect();
        const clientX = e.clientX ?? e.touches?.[0].clientX;
        const clientY = e.clientY ?? e.touches?.[0].clientY;
        this.x = (clientX - r.left) / this.zoomRef.value;
        this.y = (clientY - r.top) / this.zoomRef.value;
      }
    
      pos() {
        return { x: this.x, y: this.y };
      }
    }

   this can then be initialized in a global context and update itself as the mouse moves, which (should) compute the mouse position eagerly and make it faster to access when needed. (It also suggested adding a 'touchmove' listener, so initialization would look something like

   const zoomState = { value: 1 };
   const pointer = new Pointer(stage, zoomState);
   
   workspace.addEventListener('pointermove', e => pointer.update(e));
   workspace.addEventListener('touchmove', e => pointer.update(e));

4. DragController
   This class would bring together the drag mechanics into one place, and uses the other three to simplify. This example seems like it will need more modification than the others, but is still a useful theoretical example

   class DragController {
     constructor(pointer) {
       this.pointer = pointer;
       this.dragging = null;
       this.start = null;
   
       document.addEventListener('pointermove', e => this.onMove(e));
       document.addEventListener('pointerup', e => this.onUp(e));
     }
   
     beginDrag(box) {
       this.start = {
         px: this.pointer.x,
         py: this.pointer.y,
         x: box.x,
         y: box.y
       };
       this.dragging = box;
     }
   
     onMove(e) {
       if (!this.dragging) return;
       this.pointer.update(e);
       const dx = this.pointer.x - this.start.px;
       const dy = this.pointer.y - this.start.py;
       this.dragging.moveTo(this.start.x + dx, this.start.y + dy);
     }
   
     onUp(e) {
       if (!this.dragging) return;
       // You can add snapping / occupancy checks here
       this.dragging = null;
       this.start = null;
     }
   }

   And then launcher creation is just

   $launcher.on('pointerdown', e => {
     pointer.update(e);
     drag.beginDrag(model); // model is a Box instance
   });

I know there is a lot there but I think this will break it up into the right steps and make the flow of the code a lot clearer. I do like the functionality as it stands, so you might consider writing tests before these changes just to keep parity with what you have now. That being said testing this will be tough, but it will help later on if we already have some rudimentary ones ensuring everything moves around properly.

[Bubballoo3]

Also forgot to mention that it looks like these changes conflict with an existing PM test (see CI logs), so that will have to be resolved before we can merge

[Bubballoo3]

Another thing I just noticed (which the refactoring may solve, or at least make easier to solve) is that while the edges track very well with the mouse, the boxes 'stick' as you pull them across. Everything makes it to the right place eventually, but it would be nice if we set up the listeners so that both the arrow and box move in sync.

[harshit-soora]

The tests are fixed now, all review changes have been taken care of.

[Bubballoo3]

Ok I dug into this and figured out what was making the launchers stick. When you set the endpoints of the SVG lines, these changes are rendered synchronously, but changing the style of the html element works asynchronously as it doesn't render until the CSS reloads. The way to fix this is by using style.transform instead. I tested this by replacing Box#moveTo with

  moveTo(x, y) {
    this.x = x;
    this.y = y;
    this.el.style.transform = `translate(${x}px, ${y}px)`;
  }  

Which makes the movement smooth. Unfortunately this seems to do something with the initial position, and renders the box one grid place to the right (despite edges acting totally normal, pointing to the position the box should be). I think if you can finish implementing that and work out the issues this should be ready to merge.

[Bubballoo3]

@johrstrom I think this is ready for your review, there is still going to be more work on the workflows file in future PRs as we integrate this into the backend, but I am happy at the moment with the mechanics, organization and maintainability of what we have.

@harshit-soora until Jeff gets to this, (and I hate to add one more thing again) there seems to be a bug if you double click the 'create launcher button', where it creates duplicate overlapping launchers and generally breaks it (because they have the same id). So we should disable the button during creation to prevent this.

I also noticed that the 'connect launchers' button stays clicked until it is pressed again instead of resetting after an edge is made. I could go either way on this behavior, it does make it easy to connect everything quickly but also leads to behavior that seems buggy but is actually just because you don't realize it is still on. Either way I wanted to document that here for discussion. Nice work!

[harshit-soora]

seems buggy but is actually just because you don't realize it is still on

I added button accents (even widen the border) so it is more prominent that it has been clicked.

I tried having feature where you have to click it everytime you are connecting an edge. To me it felt like a lot of labour to click the button over and over.