From 63ac7287321a101c3f601eeb151be73154ef7720 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Wed, 11 May 2022 12:13:25 +0200
Subject: [PATCH] Web: remove concept of "selected jobs" and replace with
 "active job"

The selection mechanism of Tabulator was getting in the way of having nice
navigation, as it would deselect (i.e. nav to "/") before selecting the
next job (i.e. nav to "/jobs/{job-id}").

The active job is now determined by the URL and thus handled by Vue Router.
Clicking on a job simply navigates to its URL, which causes the reactive
system to load & display it.

It is still intended to get job selection for "mass actions", but that's
only possible after normal navigation is working well.
---
 web/app/src/App.vue                      |  8 +--
 web/app/src/assets/base.css              | 12 ++++
 web/app/src/components/JobActionsBar.vue |  3 +-
 web/app/src/components/JobsTable.vue     | 72 +++++++++++++++---------
 web/app/src/main.js                      |  2 +-
 web/app/src/router/index.js              | 15 +++--
 web/app/src/stores/jobs.js               | 39 +++++--------
 web/app/src/urls.js                      |  4 +-
 web/app/src/views/IndexView.vue          | 46 +++++++++++++++
 web/app/src/views/JobsView.vue           | 70 +++++++++++++++--------
 10 files changed, 182 insertions(+), 89 deletions(-)
 create mode 100644 web/app/src/views/IndexView.vue

diff --git a/web/app/src/App.vue b/web/app/src/App.vue
index 1e7b6dd4..e993fdaf 100644
--- a/web/app/src/App.vue
+++ b/web/app/src/App.vue
@@ -1,16 +1,16 @@
 <template>
   <header>
-    <a href="/" class="navbar-brand">{{ flamencoName }}</a>
+    <router-link :to="{ name: 'index' }" class="navbar-brand">{{ flamencoName }}</router-link>
     <nav>
       <ul>
         <li>
-          <router-link to="/">Jobs</router-link>
+          <router-link :to="{ name: 'jobs' }">Jobs</router-link>
         </li>
         <li>
-          <router-link to="/workers">Workers</router-link>
+          <router-link :to="{ name: 'workers' }">Workers</router-link>
         </li>
         <li>
-          <router-link to="/settings">Settings</router-link>
+          <router-link :to="{ name: 'settings' }">Settings</router-link>
         </li>
       </ul>
     </nav>
diff --git a/web/app/src/assets/base.css b/web/app/src/assets/base.css
index 5830e5cf..46be8d2c 100644
--- a/web/app/src/assets/base.css
+++ b/web/app/src/assets/base.css
@@ -335,3 +335,15 @@ footer {
 .status-cancel-requested {
   background-color: transparent;
 }
+
+.tabulator-row {
+  cursor: pointer;
+}
+
+.tabulator-row.active-row {
+  font-weight: bold;
+  background-color: #9900ff44;
+}
+.tabulator-row.active-row.tabulator-row-even {
+  background-color: #a92cfd44;
+}
diff --git a/web/app/src/components/JobActionsBar.vue b/web/app/src/components/JobActionsBar.vue
index 1e245ae5..fd90fe53 100644
--- a/web/app/src/components/JobActionsBar.vue
+++ b/web/app/src/components/JobActionsBar.vue
@@ -33,7 +33,8 @@ export default {
     },
 
     _handleJobActionPromise(promise, description) {
-      const numJobs = this.jobs.numSelected;
+      // const numJobs = this.jobs.numSelected;
+      const numJobs = 1;
       return promise
         .then(() => {
           let message;
diff --git a/web/app/src/components/JobsTable.vue b/web/app/src/components/JobsTable.vue
index 26ed0dbf..1d8e2ead 100644
--- a/web/app/src/components/JobsTable.vue
+++ b/web/app/src/components/JobsTable.vue
@@ -1,7 +1,6 @@
 <template>
   <div>
     <h2 class="column-title">Jobs</h2>
-
     <job-actions-bar />
     <div class="job-list" id="flamenco_job_list"></div>
   </div>
@@ -17,7 +16,9 @@ import { apiClient } from '@/stores/api-query-count';
 import JobActionsBar from '@/components/JobActionsBar.vue'
 
 export default {
-  emits: ["selectedJobChange"],
+  name: 'JobsTable',
+  props: ["activeJobID"],
+  emits: ["tableRowClicked"],
   components: {
     JobActionsBar,
   },
@@ -25,7 +26,11 @@ export default {
     const options = {
       // See pkg/api/flamenco-manager.yaml, schemas Job and JobUpdate.
       columns: [
-        { formatter: "rowSelection", titleFormatter: "rowSelection", hozAlign: "center", headerHozAlign: "center", headerSort: false },
+        // { formatter: "rowSelection", titleFormatter: "rowSelection", hozAlign: "center", headerHozAlign: "center", headerSort: false },
+        {
+          title: "ID", field: "id", headerSort: false,
+          formatter: (cell) => cell.getData().id.substr(0, 8),
+        },
         {
           title: 'Status', field: 'status', sorter: 'string',
           formatter(cell, formatterParams) { // eslint-disable-line no-unused-vars
@@ -53,7 +58,7 @@ export default {
         { column: "updated", dir: "desc" },
       ],
       data: [], // Will be filled via a Flamenco API request.
-      selectable: 1, // Only allow a single row to be selected at a time.
+      selectable: false, // The active job is tracked by click events, not row selection.
     };
     return {
       options: options,
@@ -64,11 +69,30 @@ export default {
     // jobsTableVue.processJobUpdate({id: "ad0a5a00-5cb8-4e31-860a-8a405e75910e", status: "heyy", updated: DateTime.local().toISO(), previous_status: "uuuuh", name: "Updated manually"});
     // jobsTableVue.processJobUpdate({id: "ad0a5a00-5cb8-4e31-860a-8a405e75910e", status: "heyy", updated: DateTime.local().toISO()});
     window.jobsTableVue = this;
+
+    // Set the `rowFormatter` here (instead of with the rest of the options
+    // above) as it needs to refer to `this`, which isn't available in the
+    // `data` function.
+    this.options.rowFormatter = (row) => {
+      const data = row.getData();
+      const isActive = (data.id === this.activeJobID);
+      row.getElement().classList.toggle("active-row", isActive);
+    };
     this.tabulator = new Tabulator('#flamenco_job_list', this.options);
-    this.tabulator.on("rowSelected", this.onRowSelected);
-    this.tabulator.on("rowDeselected", this.onRowDeselected);
+    this.tabulator.on("rowClick", this.onRowClick);
     this.tabulator.on("tableBuilt", this.fetchAllJobs);
   },
+  watch: {
+    activeJobID(newJobID, oldJobID) {
+      this._reformatRow(oldJobID);
+      this._reformatRow(newJobID);
+    },
+  },
+  computed: {
+    selectedIDs() {
+      return this.tabulator.getSelectedData().map((job) => job.id);
+    }
+  },
   methods: {
     onReconnected() {
       // If the connection to the backend was lost, we have likely missed some
@@ -91,7 +115,6 @@ export default {
       // "Down-cast" to JobUpdate to only get those fields, just for debugging things:
       // data.jobs = data.jobs.map((j) => API.JobUpdate.constructFromObject(j));
       this.tabulator.setData(data.jobs);
-      this._restoreRowSelection();
     },
     processJobUpdate(jobUpdate) {
       // updateData() will only overwrite properties that are actually set on
@@ -104,27 +127,22 @@ export default {
         .then(this.sortData);
     },
 
-    // Selection handling.
-    onRowSelected(selectedRow) {
-      const selectedData = selectedRow.getData();
-      this._storeRowSelection([selectedData]);
-      this.$emit("selectedJobChange", selectedData);
-    },
-    onRowDeselected(deselectedRow) {
-      this._storeRowSelection([]);
-      this.$emit("selectedJobChange", null);
-    },
-    _storeRowSelection(selectedData) {
-      const selectedJobIDs = selectedData.map((row) => row.id);
-      localStorage.setItem("selectedJobIDs", selectedJobIDs);
-    },
-    _restoreRowSelection() {
-      const selectedJobIDs = localStorage.getItem('selectedJobIDs');
-      if (!selectedJobIDs) {
-        return;
-      }
-      this.tabulator.selectRow(selectedJobIDs);
+    onRowClick(event, row) {
+      // Take a copy of the data, so that it's decoupled from the tabulator data
+      // store. There were some issues where navigating to another job would
+      // overwrite the old job's ID, and this prevents that.
+      const rowData = plain(row.getData());
+      this.$emit("tableRowClicked", rowData);
     },
+
+    _reformatRow(jobID) {
+      // Use tab.rowManager.findRow() instead of `tab.getRow()` as the latter
+      // logs a warning when the row cannot be found.
+      const row = this.tabulator.rowManager.findRow(jobID);
+      if (!row) return
+      if (row.reformat) row.reformat();
+      else if (row.reinitialize) row.reinitialize(true);
+    }
   }
 };
 </script>
diff --git a/web/app/src/main.js b/web/app/src/main.js
index 6253d57e..1cd18e55 100644
--- a/web/app/src/main.js
+++ b/web/app/src/main.js
@@ -9,7 +9,7 @@ import router from '@/router/index'
 import { DateTime } from 'luxon';
 window.DateTime = DateTime;
 
-// Help with debugging. This removes any Vue reactivity.
+// This removes any Vue reactivity.
 window.plain = (x) => { return JSON.parse(JSON.stringify(x)) };
 
 const app = createApp(App)
diff --git a/web/app/src/router/index.js b/web/app/src/router/index.js
index 5549590d..2bc5188e 100644
--- a/web/app/src/router/index.js
+++ b/web/app/src/router/index.js
@@ -5,11 +5,14 @@ const router = createRouter({
   routes: [
     {
       path: '/',
+      name: 'index',
+      component: () => import('../views/IndexView.vue'),
+    },
+    {
+      path: '/jobs/:jobID?',
       name: 'jobs',
-      // route level code-splitting
-      // this generates a separate chunk (ViewName.[hash].js) for this route
-      // which is lazy-loaded when the route is visited.
-      component: () => import('../views/JobsView.vue')
+      component: () => import('../views/JobsView.vue'),
+      props: true,
     },
     {
       path: '/workers',
@@ -20,8 +23,8 @@ const router = createRouter({
       path: '/settings',
       name: 'settings',
       component: () => import('../views/SettingsView.vue')
-    }
-  ]
+    },
+  ],
 })
 
 export default router
diff --git a/web/app/src/stores/jobs.js b/web/app/src/stores/jobs.js
index 7f8daee8..f22c2c64 100644
--- a/web/app/src/stores/jobs.js
+++ b/web/app/src/stores/jobs.js
@@ -10,8 +10,6 @@ const jobsAPI = new API.JobsApi(apiClient);
 // See https://pinia.vuejs.org/core-concepts/
 export const useJobs = defineStore('jobs', {
   state: () => ({
-    /** @type {API.Job[]} */
-    selectedJobs: [],
     /** @type {API.Job} */
     activeJob: null,
     /**
@@ -21,9 +19,6 @@ export const useJobs = defineStore('jobs', {
     activeJobID: "",
   }),
   getters: {
-    numSelected() {
-      return this.selectedJobs.length;
-    },
     canDelete() {
       return this._anyJobWithStatus(["queued", "paused", "failed", "completed", "canceled"])
     },
@@ -35,25 +30,20 @@ export const useJobs = defineStore('jobs', {
     },
   },
   actions: {
-    // Selection of jobs.
-    setSelectedJob(job) {
+    setActiveJobID(jobID) {
+      this.$patch({
+        activeJob: {id: jobID},
+        activeJobID: jobID,
+      });
+    },
+    setActiveJob(job) {
       this.$patch({
-        selectedJobs: [job],
         activeJob: job,
         activeJobID: job.id,
       });
     },
-    setSelectedJobs(jobs) {
-      const activeJob =jobs[jobs.length-1]; // Last-selected is the active one.
-      this.$patch({
-        selectedJobs: jobs,
-        activeJob: activeJob,
-        activeJobID: activeJob.id,
-      });
-    },
     deselectAllJobs() {
       this.$patch({
-        selectedJobs: [],
         activeJob: null,
         activeJobID: "",
       });
@@ -67,12 +57,6 @@ export const useJobs = defineStore('jobs', {
      * TODO: actually have these work on all selected jobs. For simplicity, the
      * code now assumes that only the active job needs to be operated on.
      */
-    deleteJobs() {
-      const deletionPromise = new Promise( (resolutionFunc, rejectionFunc) => {
-        rejectionFunc({code: 327, message: "deleting jobs is not implemented in JS yet"});
-      });
-      return deletionPromise;
-    },
     cancelJobs() { return this._setJobStatus("cancel-requested"); },
     requeueJobs() { return this._setJobStatus("requeued"); },
 
@@ -84,7 +68,8 @@ export const useJobs = defineStore('jobs', {
      * @returns bool indicating whether there is a selected job with any of the given statuses.
      */
     _anyJobWithStatus(statuses) {
-      return this.selectedJobs.reduce((foundJob, job) => (foundJob || statuses.includes(job.status)), false);
+      return !!this.activeJob && statuses.includes(this.activeJob.status);
+      // return this.selectedJobs.reduce((foundJob, job) => (foundJob || statuses.includes(job.status)), false);
     },
 
     /**
@@ -93,8 +78,12 @@ export const useJobs = defineStore('jobs', {
      * @returns a Promise for the API request.
      */
     _setJobStatus(newStatus) {
+      if (!this.activeJobID) {
+        console.warn(`_setJobStatus(${newStatus}) impossible, no active job ID`);
+        return;
+      }
       const statuschange = new API.JobStatusChange(newStatus, "requested from web interface");
-      return jobsAPI.setJobStatus(this.activeJob.id, statuschange);
+      return jobsAPI.setJobStatus(this.activeJobID, statuschange);
     },
   },
 })
diff --git a/web/app/src/urls.js b/web/app/src/urls.js
index 8bfbd283..24da0260 100644
--- a/web/app/src/urls.js
+++ b/web/app/src/urls.js
@@ -12,8 +12,8 @@ const URLs = {
   ws: websocketURL,
 };
 
-console.log("Flamenco API:", URLs.api);
-console.log("Websocket   :", URLs.ws);
+// console.log("Flamenco API:", URLs.api);
+// console.log("Websocket   :", URLs.ws);
 
 export function ws() {
   return URLs.ws;
diff --git a/web/app/src/views/IndexView.vue b/web/app/src/views/IndexView.vue
new file mode 100644
index 00000000..750183cf
--- /dev/null
+++ b/web/app/src/views/IndexView.vue
@@ -0,0 +1,46 @@
+<template>
+  <div class="col col-1">
+    <router-link :to="{ name: 'jobs' }"><span>J</span></router-link>
+  </div>
+  <div class="col col-2">
+    <router-link :to="{ name: 'workers' }"><span>W</span></router-link>
+  </div>
+  <div class="col col-3">
+    <router-link :to="{ name: 'settings' }"><span>S</span></router-link>
+  </div>
+  <footer>
+    Make your choice.
+  </footer>
+</template>
+
+<script>
+export default {
+  name: 'IndexView',
+  components: {},
+}
+</script>
+
+<style scoped>
+.col a {
+  display: flex;
+  align-items: center;
+  justify-content: center;
+  width: 100%;
+  height: 100%;
+
+  transition: 500ms;
+  background-color: var(--color-background-column);
+}
+
+.col a:hover {
+  text-decoration: none;
+  background-color: var(--color-accent-background);
+}
+
+.col a span {
+  font-weight: bold;
+  font-size: 10rem;
+  text-align: center;
+  vertical-align: middle;
+}
+</style>
diff --git a/web/app/src/views/JobsView.vue b/web/app/src/views/JobsView.vue
index 9a5fecac..abe6089a 100644
--- a/web/app/src/views/JobsView.vue
+++ b/web/app/src/views/JobsView.vue
@@ -1,18 +1,17 @@
 <template>
   <div class="col col-1">
-    <jobs-table ref="jobsTable" @selectedJobChange="onSelectedJobChanged" />
+    <jobs-table ref="jobsTable" :activeJobID="jobID" @tableRowClicked="onTableJobClicked" />
   </div>
   <div class="col col-2">
     <job-details :jobData="jobs.activeJob" />
-    <tasks-table v-if="jobs.activeJobID" ref="tasksTable" :jobID="jobs.activeJobID"
-      @selectedTaskChange="onSelectedTaskChanged" />
+    <tasks-table v-if="jobID" ref="tasksTable" :jobID="jobID" @selectedTaskChange="onSelectedTaskChanged" />
   </div>
   <div class="col col-3">
     <task-details :taskData="tasks.activeTask" />
   </div>
   <footer>
     <notification-bar />
-    <update-listener ref="updateListener" :websocketURL="websocketURL" :subscribedJob="jobs.activeJobID"
+    <update-listener ref="updateListener" :websocketURL="websocketURL" :subscribedJob="jobID"
       @jobUpdate="onSioJobUpdate" @taskUpdate="onSioTaskUpdate" @message="onChatMessage"
       @sioReconnected="onSIOReconnected" @sioDisconnected="onSIODisconnected" />
   </footer>
@@ -34,6 +33,7 @@ import UpdateListener from '@/components/UpdateListener.vue'
 
 export default {
   name: 'JobsView',
+  props: ["jobID"], // provided by Vue Router.
   components: {
     JobsTable, JobDetails, TaskDetails, TasksTable, NotificationBar, UpdateListener,
   },
@@ -46,25 +46,18 @@ export default {
   }),
   mounted() {
     window.jobsView = this;
+    this._fetchJob(this.jobID);
+  },
+  watch: {
+    jobID(newJobID, oldJobID) {
+      this._fetchJob(newJobID);
+    },
   },
   methods: {
-    // onSelectedJobChanged is called whenever the selected job changes; this is
-    // both when another job is selected and when the selected job itself gets
-    // updated.
-    onSelectedJobChanged(jobSummary) {
-      if (!jobSummary) { // There is no selected job.
-        this.jobs.deselectAllJobs();
-        return;
-      }
-
-      const jobsAPI = new API.JobsApi(apiClient);
-      jobsAPI.fetchJob(jobSummary.id)
-        .then((job) => {
-          this.jobs.setSelectedJob(job);
-          // Forward the full job to Tabulator, so that that gets updated too.
-          this.$refs.jobsTable.processJobUpdate(job);
-        });
+    onTableJobClicked(rowData) {
+      this._routeToJob(rowData.id);
     },
+
     onSelectedTaskChanged(taskSummary) {
       if (!taskSummary) { // There is no selected task.
         this.tasks.deselectAllTasks();
@@ -95,8 +88,8 @@ export default {
         console.warn("App: this.$refs.jobsTable is", this.$refs.jobsTable);
       }
 
-      if (this.jobs.activeJobID == jobUpdate.id) {
-        this.onSelectedJobChanged(jobUpdate);
+      if (this.jobID == jobUpdate.id) {
+        this._fetchJob(jobUpdate.id);
       }
     },
     onJobNew(jobUpdate) {
@@ -109,6 +102,32 @@ export default {
       this.$refs.jobsTable.processNewJob(jobUpdate);
     },
 
+    /**
+     * @param {string} jobID job ID to navigate to, can be empty string for "no active job".
+     */
+    _routeToJob(jobID) {
+      this.$router.push({ name: 'jobs', params: { jobID: jobID } });
+    },
+
+    /**
+     * Fetch job info and set the active job once it's received.
+     * @param {string} jobID job ID, can be empty string for "no job".
+     */
+    _fetchJob(jobID) {
+      if (!jobID) {
+        this.jobs.deselectAllJobs();
+        return;
+      }
+
+      const jobsAPI = new API.JobsApi(apiClient);
+      return jobsAPI.fetchJob(jobID)
+        .then((job) => {
+          this.jobs.setActiveJob(job);
+          // Forward the full job to Tabulator, so that that gets updated too.
+          this.$refs.jobsTable.processJobUpdate(job);
+        });
+    },
+
     /**
      * Event handler for SocketIO task updates.
      * @param {API.SocketIOTaskUpdate} taskUpdate
@@ -132,8 +151,13 @@ export default {
         this.$refs.tasksTable.onReconnected();
     },
     onSIODisconnected(reason) {
-      this.jobs.deselectAllJobs();
     },
   },
 }
 </script>
+
+<style scoped>
+.isFetching {
+  opacity: 50%;
+}
+</style>