Desktop environment paths and readonly mode by f4str · Pull Request #852 · Kitware/dive

f4str · 2021-07-20T18:53:54Z

fixes #640
fixes #727

Added environment overrides on desktop under the DIVE_VIAME_INSTALL_PATH and DIVE_READONLY_MODE environment variables. If they are provided, they will override the settings for viamePath and readonlyMode respectively. If the environment variable DIVE_VIAME_INSTALL_PATH is provided, it will also prevent the user from changing the path in any way:

Implemented read only mode on desktop. This can be either toggled on/off from the settings or the DIVE_READONLY_MODE environment variable can be provided. Even if this variable is provided, the user can still toggle it on or off. When read only mode is active, a warning will be displayed in the settings:

In read only mode, the user is able to make changes as usual, however, they cannot save any changes. The save icon will be disabled with a warning logo to catch attention:

When hovering over the save icon, it will display a descriptive message:

However, the Run Pipeline and Import functionality still work (as requested) which can still modify the data even in read only mode. Although this is not good in terms of the application perspective, this is still the desired feature.

Fixed the issue with "fake" saving on the settings page on desktop. Previously when changing any settings (such as the viame path), the desktop app would update immediately before clicking the save option. When navigating away to a different page, these changes would persist. However, when reloading the app, these changes would not truly save until the save button was actually clicked. Now, when making local changes, nothing will update until clicking the save button. Navigating away to a different page will also discard any unsaved changes on the settings page.

Read only mode is only for desktop, it is not implemented for web.

…ides

BryonLewis

Couple of things primarily related to adding environment variables after launching it without them. Only other thing is either instructions or options if the install path is disabled as well as if it is disabled and the environment path is invalid.

BryonLewis · 2021-07-21T19:14:19Z

client/platform/desktop/frontend/store/settings.ts

+        ...settingsValue,
+        // Overwrite with explicitly persisted settings
+        ...maybeSettings,


This could be a two-part because the correct localSettings aren't written unless you save the data So see the other comment further down.
If you don't have a DIVE_VIAME_INSTALL_PATH set and have a previous version when you load up the data it will set override to {} in the local settings.
If you then set DIVE_VIAME_INSTALL_PATH afterwards and load it up, the locaStorage will always have the overrides: {}. I'm guessing because your spreading the ...maybeSettings over the top of ...settingsValue so it is wiping out the default overrides: with the blank one.
Even if you get the correct settings set here it doesn't update the localSettings for the client. I don't know if that is required during this part.

I was a bit confused what you meant at first, but after understanding this is a great catch. Really appreciate you pointing out this major issue. I was actually having trouble with getting the environment variables to load in and thought I was going insane, but this was actually the issue.

A quick temporary fix I'm trying for now is just to delete the overrides property from maybeSettings (since it's a JSON and not a proper type). Then spreading it on top of settingsValue will overwrite everything except the overrides. I'm open to any better solutions.

I think you might want to check the overrides to ensure the Object.keys(maybeSettings).length !== 0 or something similar before deleting it or setting it to undefined. If there exists the path/readonly settings you probably want them to spread and overwrite the defaults.

Could we add a little comment above the delete line about what that's for?

BryonLewis · 2021-07-21T19:15:22Z

client/platform/desktop/frontend/store/settings.ts

+    settingsValue.readonlyMode = settingsValue.overrides.readonlyMode;
+  }
+  settings.value = settingsValue;
  ipcRenderer.send('update-settings', settings.value);


I don't know exactly if this would be a incorrect/correct option but if the settings are different than the base localSettings like the case where there is an environment variable. Here is where I think you would want to just call updateSettings from further down to update the localStorage as well

BryonLewis · 2021-07-21T19:17:03Z

client/platform/desktop/frontend/components/Settings.vue

+                :disabled="!!(localSettings.overrides && localSettings.overrides.viamePath)"
                persistent-hint
              />
            </v-col>
+
            <v-col cols="3">
              <v-btn
                large
                block
                color="primary"
-                class="mb-6"
+                :disabled="!!(localSettings.overrides && localSettings.overrides.viamePath)"


I think these two need some explicit text stating why they are disabled. Something about the environment variable name being set and using that instead of manually allowing changing.

Good point. I'll add an info box saying the environment exists and changes are being locked.

client/platform/desktop/frontend/components/Settings.vue

f4str · 2021-07-22T15:45:15Z

The save button is now disabled when there are no pending changes. This is just a simple computed ref checking if the local settings match the global settings.

Fixed the major issue where the overrides: {} property from local settings can overwrite the actual overrides property from environment variables. For now a simple solution is to delete the overrides property from the local settings json since that's the only problematic property. Now running

DIVE_VIAME_INSTALL_PATH=something yarn serve:electron

will properly cause the path to be set and will lock changes.

Added an info box to indicate the DIVE_VIAME_INSTALL_PATH environment variable is set and that changes are locked. For now its placed right under the kwiver initialization message, but putting on the top (before the GPU message) might be better. I'm open to any suggestions for placement and text.

f4str · 2021-07-22T15:51:10Z

There are also a few outstanding points of concern

As pointed out before, should the VIAME install path folder option still be disabled if the environment variable is provided but is a bad path
Should the readonly mode switch be disabled if the environment variable is provided.
There is a race condition between loading settings and initializing settings if reloading the electron app on the settings page (ctrl + R). Looks something like this

where the settings aren't initialized and everything is empty. It's fixed by going to a different page and back. This isn't caused by this PR and has always been an issue (same issue on the main branch), but should probably be addressed now rather than later.

subdavis

LGTM, tested and works quite well!

BryonLewis

Nice, looks good.

subdavis

One quick question about a command reorder.

subdavis · 2021-07-27T12:49:53Z

client/platform/desktop/frontend/components/Settings.vue

    onBeforeMount(async () => {
-      settingsAreValid.value = await validateSettings(localSettings.value);
      smi.value = await nvidiaSmi();
+      settingsAreValid.value = await validateSettings(localSettings.value);


You flipped the order here. Does this change anything? If this order matters, you have a race condition that needs to be addressed.

f4str · 2021-07-27T14:26:13Z

After discussion, there is no longer a race condition and entry points of error in Settings.vue so the two lines can be swapped back to their original order.

Also updated the desktop settings screenshot for the docs.

subdavis and others added 9 commits July 13, 2021 09:42

Env overrides

daa1438

Merge branch 'main' of https://github.com/Kitware/dive into env-overr…

442f06e

…ides

started functionality on readonly mode for desktop

e565a61

Merge branch 'main' of https://github.com/Kitware/dive into env-overr…

5008f2d

…ides

Created readonly mode visuals on viewer

9479c7f

Created switch for readonly mode on desktop settings page

f81bced

started functionality for readonly mode on web

4275c41

finalized desktop readonly mode

a954b08

Merge branch 'main' of https://github.com/Kitware/dive into env-overr…

fcbee14

…ides

f4str requested review from BryonLewis and subdavis July 20, 2021 18:53

fixed uninitialized settings crash

e493240

BryonLewis requested changes Jul 21, 2021

View reviewed changes

Farhan Ahmed added 2 commits July 22, 2021 10:33

added info box about viame path environment variable

95fe478

fixed environment variable overrides

6414c86

subdavis force-pushed the env-overrides branch from c3a5265 to 6414c86 Compare July 26, 2021 19:19

Report true null, fix race conditions (#857)

1507434

subdavis previously approved these changes Jul 26, 2021

View reviewed changes

BryonLewis previously approved these changes Jul 26, 2021

View reviewed changes

subdavis dismissed their stale review via 1507434 July 26, 2021 21:35

cleanup and docs update

0d2a096

f4str dismissed BryonLewis’s stale review via 0d2a096 July 26, 2021 21:37

BryonLewis previously approved these changes Jul 27, 2021

View reviewed changes

subdavis previously approved these changes Jul 27, 2021

View reviewed changes

updated desktop settings screenshot in docs

16cae13

f4str dismissed stale reviews from subdavis and BryonLewis via 16cae13 July 27, 2021 14:24

subdavis approved these changes Jul 27, 2021

View reviewed changes

f4str merged commit b367c2b into main Jul 27, 2021

f4str deleted the env-overrides branch July 27, 2021 14:37

Conversation

f4str commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

f4str commented Jul 22, 2021

Uh oh!

f4str commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f4str commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

f4str commented Jul 20, 2021 •

edited

Loading

f4str commented Jul 22, 2021 •

edited

Loading