Skip to content

Feature 190#891

Merged
BryonLewis merged 10 commits intomainfrom
feature-190
Sep 8, 2021
Merged

Feature 190#891
BryonLewis merged 10 commits intomainfrom
feature-190

Conversation

@BryonLewis
Copy link
Collaborator

@BryonLewis BryonLewis commented Aug 15, 2021

Fixes #190

  • Adds into the user Settings a prompt setting for prompting the user when deleting objects.
  • Restructured NewTrackSettings into having TrackSettings with a new DeletionSettings. It is one value for now but I don't know if more will be added in the future.
  • CreationMode has been renamed to TrackSettings to more accurately reflect being tracksettings
  • The localStorage uses the same format just with an additional field.
  • Updated the useModeManager handleRemoveTrack to prompt if the user settings indicate so. There is an option override which is used for track visibility deletion tool (The delete button next to track settings).

Selection_265

  • Not a huge fan of having the multiple top-level items in the storage but I didn't want to interrupt anyone's current track creation settings.

@BryonLewis BryonLewis marked this pull request as ready for review September 1, 2021 12:10
Copy link
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be vigilant about naming and avoid proliferation of symbols with names that 1) aren't very descriptive and 2) are very close to other things. There's a similar problem in common.ts in desktop.

Stuff like this really impacts readability because rather than relying on intuition, you have to rely on bulk memorization to know what things do.

Please hold me accountable to this as well.

Rather than introduce 2 more symbols to the useSettings namespace, could we just rename NewTrackSettings to TrackSettings add promptUserOnDelete to TrackSettings.

Putting DeletionSettings into its own namespace doesn't make sense, because if we end up with settings that affect other types of deletions (users, types, etc) then we can't put them in DeletionSettings because it's a child of TrackSettings.

Then, updateNewTrackSettings should move to simply updateTrackSettings.

I'd actually prefer to do away from putting settings into these arbitrary groups at all and have a AnnotatorSettings interface and a updateSettings() function and never have to add new symbols to the useSettings api, but this could be out of scope for this PR.

@BryonLewis
Copy link
Collaborator Author

I'd actually prefer to do away from putting settings into these arbitrary groups at all and have a AnnotatorSettings interface and a updateSettings() function and never have to add new symbols to the useSettings api, but this could be out of scope for this PR.

Understood and I wasn't a fan either considering my last bulletpoint. I'll swap it, I was just avoiding doing an update to users who currently had data in localStorage stored a top level newTrackSettings

@subdavis
Copy link
Contributor

subdavis commented Sep 3, 2021

I was just avoiding doing an update to users who currently had data in localStorage stored a top level newTrackSettings

On the balance, maybe don't bother? Just change the name of the variable and let users's settings be overwritten with the defaults. It's just a couple toggle switches. I don't think I'd bother attempting to migrate them.

@BryonLewis
Copy link
Collaborator Author

I need to do some double checking of stuff, and I think refactoring some of the components to composition and using toRef so I can maintain reactivity and not have these long property chains.

@BryonLewis
Copy link
Collaborator Author

Okay I think I have this working like it should. a reactive AnnotationSettings object is passed around and depending on where it is used the sub-settings are converted to a ref using toRef so they maintain their reactivity and can be used in vue templates while destructuring the property.
This keeps everything using the type of AnnotationSettings and the toRef will maintain the proper type with the wrapper of a Ref<SubSettingsType so it can be properly used.

@subdavis subdavis mentioned this pull request Sep 7, 2021
* Try settings singleton

* Lint fixes

* fixing continuous v-model

* mend

Co-authored-by: Bryon Lewis <Bryon.Lewis@kitware.com>
Copy link
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one nit.

}

function handleRemoveTrack(trackIds: TrackId[]) {
async function handleRemoveTrack(trackIds: TrackId[], promptOverride = false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ambiguous boolean. Does override mean to force enable or force disable? Could this be renamed to forceEnable or forceDisable based on what it is supposed to do?

Copy link
Collaborator Author

@BryonLewis BryonLewis Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you good with forcePromptDisable? just so you know what you're force disabling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep sounds good.

@BryonLewis BryonLewis merged commit 2629e4e into main Sep 8, 2021
@subdavis subdavis deleted the feature-190 branch September 8, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Once user settings are implemented add Delete confirmation Setting

2 participants