force-h2020 / force-wfmanager

Workflow manager: UI application for Business Decision System
Other
1 stars 0 forks source link

ToolBar items duplicated in both WfManagerReviewTask and WfManagerSetupTask #371

Closed flongford closed 4 years ago

flongford commented 4 years ago

Currently the tool bar in both review and setup task contains duplicated code, which needs to be updated whenever any objects change state during run time.

Using the envisage TasksExtensions API to only generate one instance of each unique element in the tool bar would be desirable, and simplify the icon state updates (enabled/disabled during an MCO run).

https://docs.enthought.com/envisage/tasks_user_manual/extensibility.html

sparsonslab commented 4 years ago

Surely there shouldn't be any duplicated code in the first place? i.e. the run, stop and pause elements of the toolbar should only appear in one pane or the other, not both. At the moment there are actually three (!) run buttons: in both panes and at the bottom of the window (dock pane?).

Why not just delete the duplicated toolbar elements?

So for example,

WfManagerSetupTask._tool_bars_default() TaskAction( name="Run", tooltip="Run Workflow", image=ImageResource("baseline_play_arrow_black_48dp"), method="run_bdss", enabled_name="run_enabled", image_size=(64, 64), ),

WfManagerReviewTask._tool_bars_default() TaskAction( name="Run", tooltip="Run Workflow", image=ImageResource("baseline_play_arrow_black_48dp"), method="setup_task.run_bdss", enabled_name="run_enabled", image_size=(64, 64), ),

The review run just calls the setup pane run. And the same for stop, pause, plugin. Just delete the review run/stop/pause/plugin?

flongford commented 4 years ago

From a UX perspective, it's convenient to be able to control the run from anywhere in the software (not having to keep switching back to the setup pane).

We also have other duplications of code / shared state between both tasks so it would be beneficial to investigate a way to tidying these up.

Note: the task here isn't to change the software, but to come up with a better way (if possible) of performing the same functionality,

flongford commented 4 years ago

_Note: This solution builds on the following commit that suggests how to get the enabled_name references to work:_ https://github.com/force-h2020/force-wfmanager/commit/c74269ee542414a10931811bf92fdd72aa5c6c26

@sparsonslab Update after discussion with Corran:

I can't get the state changes to work for the run/pause/stop as I have no way of now accessing them the tasks. Usually,

line 525, wfmanager_setup_task

self.tool_bars[0].items[2],
self.review_task.tool_bars[0].items[2],

But now the extended run/pause/stop toolbar is not an item in self.tool_bars[] or self.review_task.tool_bars[].

We could achieve the same result (changing icon / name on a tool bar object) by having 2 toolbar objects that become visible / not visible in reaction to the same event.

This is less elegant, but is robust (if the underlying logic controlling the visible_name attributes are) and can be easily changed in the future if the UI requirements also change.

This would mean including the following in the WfManagerGlobalTask.actions list:

TaskAction(
    name="Pause",
    tooltip="Pause Workflow",
    method="setup_task.pause_bdss",
    enabled_name='setup_task.computation_running',
    visible_name='setup_task._not_paused',
    image=ImageResource("baseline_pause_black_18dp"),
    image_size=(64, 64),
),
TaskAction(
    name="Resume",
    tooltip="Resume Workflow",
    method="setup_task.pause_bdss",
    enabled_name='setup_task.computation_running',
    visible_name='setup_task._paused'
    image=ImageResource("baseline_skip_next_black_18dp.png"),
    image_size=(64, 64),
)

With the undying logic controlling setup_task._paused and setup_task._not_paused is defined in the WfManagerSetupTask class

Originally posted by @flongford in https://github.com/force-h2020/force-wfmanager/pull/374#issuecomment-613365052