Log view: use initialOptions to save & restore view state on navigation
#1688
Conversation
| // Watch id & file together: | ||
| this.$watch( | ||
| () => ({ | ||
| id: this.id ?? undefined, // (treat null as undefined) | ||
| file: this.file ?? undefined | ||
| }), | ||
| async ({ id }, old) => { | ||
| // update the query when the id or file change | ||
| this.updateQuery() | ||
| // refresh the file list when the id changes | ||
| if (id !== old?.id) { | ||
| await this.updateLogFileList() | ||
| } | ||
| }, | ||
| { immediate: true } | ||
| ) |
There was a problem hiding this comment.
Rejigged the watchers for file and id to prevent race conditions. The usual order of events now is (for the example of opening the workflow log):
-
idgets set for the first time to'~user/workflow'.fileis nullish.updateQuery()gets called because theidchanged, but becausefileis nullish it doesn't really do anythingLines 429 to 431 in 58f5d96
updateLogFileList()gets called because theidchanged
-
filehas been set to the default scheduler log byupdateLogFileList()updateQuery()gets called again becausefilechanged, and causes the log file to displayupdateLogFileList()doesn't get called becauseidis unchanged
| if (this.file && !(this.file in logFiles)) { | ||
| if (this.file && !logFiles.includes(this.file)) { |
There was a problem hiding this comment.
x in arr does not do what you think it does!
src/views/initialOptions.js
Outdated
| export const useInitialOptions = (name, { props, emit }) => { | ||
| const _ref = ref(props.initialOptions[name]) | ||
| watch( | ||
| _ref, | ||
| (val, old) => emit( | ||
| updateInitialOptionsEvent, | ||
| { ...props.initialOptions, [name]: val } | ||
| ), | ||
| { immediate: true, deep: true } | ||
| ) | ||
| return _ref | ||
| } |
There was a problem hiding this comment.
This is similar to using a writeable computed property e.g.
computed({
get: () => props.initialOptions[name],
set: (val) => emit(
updateInitialOptionsEvent,
{ ...props.initialOptions, [name]: val }
)
})but that seemed to lead to the ref value not updating until the next tick? Maybe the watcher approach works because it is backed up by a concrete ref? Not sure
| required: false, | ||
| default: () => {} | ||
| } | ||
| initialOptions, | ||
| }, |
There was a problem hiding this comment.
None of the other views have an initialOptions prop - is this a solution that can be rolled out for table, tree and graph views as well?
There was a problem hiding this comment.
Yes, the initialOptions prop can be added to them to allow storing their view states
| emits: [ | ||
| updateInitialOptionsEvent, | ||
| ], | ||
|
|
There was a problem hiding this comment.
in the other views I think the initialOptions logic is inside src/components/cylc/view-name/view-name.vue
Should this be the case for log view as well for consistency?
There was a problem hiding this comment.
It is the views (in src/views/) that we mount in tabs in the workspace, so these are where we want the initialOptions prop to live.
You have correctly pointed out that in some cases, the component (in src/components) contains the state we want to save per view. We might need to refactor those to extract the state into the view, which would be passed into the component as a prop (probably using v-model).
There was a problem hiding this comment.
On the split between src/views and src/components. IMO:
Views are cylc-ui plugins, the view code should contain all the boiler plate for the plugin, e.g. the views name & icon, the logic for subscription handling, mappings onto the data store, user facing options and their defaults.
Components are generic reusable items that could, conceivably, find application in multiple views. They should contain the logic for their own rendering but be as ignorant as possible of the cylc-ui application itself (but will have to make assumptions about the format of the data they are provided).
This is saved along with the Lumino layout for restoring after navigation. Implemented for the log view only so far.
| .invoke('val') | ||
| .should('eq', jobFile) | ||
| // Navigate away | ||
| cy.visit('/#/workspace/two') |
There was a problem hiding this comment.
Would we expect the state to persist when changing the route like this?
I would have thought this would cause all state to be lost unless stored in localstorage
There was a problem hiding this comment.
The state is stored in the vuex store, which persists for the lifetime of the browser session until the user refreshes the page. This is because we handle navigation using vue-router so the app is continuously up while you navigate within it
|
If the commits are similar enough that it makes sense to squash, or there is only 1 commit, or the commits are not tidy. Otherwise in this case I think the granularity of the commits makes sense for a normal merge |
Follow-up to #1664
Moved from MetRonnie#7
The view state (filters, inputs, toggles etc.) can now be saved with the Lumino tab layout by using a prop called
initialOptions.This is only implemented for the log view for now.
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.