Client/attribute ui by BryonLewis · Pull Request #427 · Kitware/dive

BryonLewis · 2020-11-06T13:51:41Z

This will use the global dataset but set up the initial stage for switching over and making it a smoother transition.
Fixes #437
Old Style:

New Style:

Goals:

Easier to view (List view with clicking on an attribute to edit)
Easier to stay in this mode while looking at multiple tracks/detections
Detections can be created/deleted from this panel (Click the settings icon to enter edit mode and delete, modify or create new attributes)
Streamline attributes for eventual dataset level attribute information stored in the metadata

TODO:

Possible Questions and some explanations for feedback:

The Track item was modified for a single display tool for the attribute editor at the top, this was to change some behavior it had.
ConfidencePairs, adding/editing and deleting was left out of this PR, but should be added in a future PR (probably once the sorting is fixed).
I can break up the AttributesPanel.vue into a subsection component something like AttributesPanelSection.vue for tracks/attributes sections because a lot of that is repeated code except for text attribute references and the frame counting.
I know there is what seems to be unnecessary error handling for the attribute creation/saving/deletion, but this is to inform the user of any eventual permission errors in the metaData versions. (I.E - a user tries to edit the attributes on someone else's dataset)
Suggestion on leaving in the OldAttributesSettings and items or should we just remove the Settings/Attributes panel from the global settings for now.
I don't know how you specifically want to do testing for vuetify components when we have platforms that implement vuetify themselves, or if you want to testing at the platform level. I figured there might need to be a discussion before doing that, but once we have a template for doing it I will create tests for these components.
Finally do we want to completely remove the older attributes editor/panel or leave it. We can leave it while we are doing the transition because it still works fine, just with renamed editors to remind myself that eventually it will be removed.

Trying to get the styling and layout setup Updating some styling Updated styling a bit and added toggling Started updating the editor and connecting it to the attributes endpoint Updating styling and fixing various issues mend mend Electron (#397) * add electron * Add missing api methods * Fix build errors for electron, add to CI * Address comments Update deploy to only Thursday Update base image for girder worker Fixes FPS issue and image caching (#419) * fixing fps and modifying cache * mend Update docker docs (#379) Co-authored-by: Brandon Davis <git@subdavis.com> Update blank.yml (#415) Upgrade to node LTS Co-authored-by: Jacob Nesbitt <jjnesbitt2@gmail.com> Add checkbuild.sh (#425) Add load and save for JSON annotations (#414) * Add load and save for JSON annotations * Relax json file name requirement * SIP * Implement settings, sanity checks, separate loading of datasets * WIP * remove settings page Fix deploy.yml Update day strings (#430) Fix typo in library README Allow setting of arbitrary attributes on video player (#435) * Mute video prop * Allow setting arbitrary attributes Downgrade urllib3 (#439) Fix inverse interpolation (#433) Run pipelines enabled incorrectly (#441) Minor fix to attributes Minor ui fixes Addressing changes Enable pinch zoom on mobile (#440) Co-authored-by: Brandon Davis <git@subdavis.com> Add dummy generators (#438) conversion to using api for attributes editing converted attribute editor and added in additional editing Updating UI interactions

subdavis

I was working on this yesterday and lost my comments, so here it goes again

Issues

Up and down arrows should navigate tracks the same way they do on the other tab, I think. This is useful as the new review mode UI.
The track list item checkbox should probably be disabled in the attribute pane.
When you move from the attribute panel back to the track list panel, if a track is selected, it isn't scrolled properly in the track list. If you tap up/down, it's corrected, but it would be nice to have it initialize properly. This is sorta unrelated to this change, but wanted to mention it.
setting the value of text and number fields I found a bit frustrating, because you have to hit "enter" for the value to save. If you hit escape or click off the focused input, the value is lost. IMO, all unfocus events should result in an update.
Changing the attribute name of an attribute that's already set will result in the value being erased for all uses of that attr. Users will find this frustrating.

UI Feedback

This is a complicated thing to get right, and I don't know what users are going to like best.

I like the ability to edit a single attribute when unused are hidden,
I'm generally unsure about how this visible/hidden toggle works.
- I've noticed that this toggle serves two somewhat conflicting purposes. 1) as a UI optimization, to hide noise and make the attribute panel look cleaner and 2) as a way of exposing the "management" interface where users can manage existing attributes. I'm concerned that users may be confused by using the same UI for both track/detection-specific editing (setting actual values) and for dataset-general editing of what attributes even exist. The co-location of these features may lead one to believe that the "options" they set for an attribute apply to a particular track. It's entirely possible that a more traditional preferences dialog would be better.
- I think the first purpose is really only a result of vuetify input boxes being so obnoxiously bloated and over-engineered. Perhaps if all these inputs were replaced with their native form equivalents, they wouldn't need to be hidden for this UI to look clean. I think the track list item is a great example of how removing the vuetify input simplified things.

All that being said, I think this represents an excellent step in the right direction. The component is really well written and can be refactored in the future if necessary.

I do have a few aesthetic suggestions, which I'll add in a PR.

subdavis · 2020-11-16T21:36:00Z

+
+<template>
+  <v-dialog
+    v-model="show"


You can't v-model with a prop, assignment will cause a warning. In general, I find it's best not to include v-dialog at all with reusable components.

I placed it outside of the component and swapped it to use :value. Don't know why I set it using v-model it was used as value calculated outside of the dialog itself anyways.

BryonLewis · 2020-11-19T19:51:28Z

I think the first purpose is really only a result of vuetify input boxes being so obnoxiously bloated and over-engineered. Perhaps if all these inputs were replaced with their native form equivalents, they wouldn't need to be hidden for this UI to look clean. I think the track list item is a great example of how removing the vuetify input simplified things.

The secondary reason is that you could have a list of disparate things that mean nothing to your specific track. If there are 20 attributes that can be set and you set index 0,1, 15 to values you then have a whole lot of information that isn't relevant for viewing if you can see all unset data at the same time. I also didn't want to resort the list of items each time based on what is set. I think this was a decent middle ground where you have your track level data and you can edit it quickly for set values, but if you want to add more you toggle the visibility (that's why it was a edit icon before).

I do like the native HTML elements more and think they look better. A bit more annoying to work with (especially the datalist for each text/value input) but it looks cleaner. The only downside is that the attribute type isn't in there anymore (it used to be the hint for the vuetify input.

subdavis · 2020-11-19T20:12:48Z

Oh, nice. I like that better too.

I think the form type provides enough info about what types are accepted, but that's just me.

BryonLewis · 2020-11-19T20:56:15Z

Not ready for review, I want to do the following before it's ready

Remove Vuetify section from AttributeInput.vue
Refactor AttributeInput.vue into composition component
AttributePanel.vue is getting too large, break it up into AttributePanelSection.vue for the Detection/Track attribute sections as well as another section for the Confidence Pairs. I know it seems like a bit much but I believe it will make it easier to modify in the long run (I think attributes may get more complicated based on some discussions)
Fix scrolling selected track issue when toggling back to the standard view

BryonLewis · 2020-11-23T14:50:06Z

I feel a bit better about this system now. I think this structure is easier to understand and to modify in the future if needed.

Swapped inputs all to native HTML elements instead of using vuetify. (Little annoying with having multiple datalist items, but it works).
I renamed the main component because it isn't really solely an Attributes Panel. Renamed it to TracksDetailsPanel.vue.
Refactored the attributes subsections into a single reusable component with mode Detection and Track. Only real different is the belongsTo key and the display of the frame numbers. I pass the core Attributes from the server through the parent object so they don't have to each make a request to the endpoint.
Added in the small trackScroll fix for the tracklist (just scrolling to the selected on mount) when switching back and forth from TrackDetailsPanel.

subdavis

Haven't quite finished. Still need to finish understanding TrackDetailsPanel.

Meanwhile, here's some comments.

Also 2 bugs I found best demonstrated by GIF.

Bug1

Options disappear in dropdown after they've been selected once

Bug2

Detections don't appear in the "clean" list after they've been saved. Save button counter goes up, but they're still invisible.

subdavis · 2020-11-20T21:24:08Z

  components: { TooltipBtn },

  props: {
+    singleDisplay: {


weird nitpick. Could we just call this solo?

singleDisplay is just awkward to me.

subdavis · 2020-11-20T21:31:07Z

+    const values: Ref<string[]> = ref(
+      props.selectedAttribute.values ? props.selectedAttribute.values : [],
+    );
+    const addNew = ref(!props.selectedAttribute._id.length);


why bother converting these to refs? You aren't the new variable's reactivity anywhere, and props is already a reactive container object.

subdavis · 2020-11-23T17:49:51Z

+            value = undefined;
+            break;
+          default:
+            value = newval;


Suggested change

value = newval;

value = newval.trim();

Probably just a good idea.

subdavis · 2020-11-23T17:51:44Z

+        switch (newval) {
+          case '':
+          case ' ':
+          case undefined:


None of these cases are possible, so this condition is unreachable code. (value evaluates false)

if (target && target.value)

yup, I added the target && target.value after some refactor afterwards when I was testing Vuetify and HTML inputs together and didn't remove this.

subdavis · 2020-11-23T18:36:25Z

+    throw new Error(`Track ${trackId} missing from map`);
+  }
+  return track;
+}


should make this a util or something...

Maybe it could be hoised from useTrackStore

I did hesitate on making a change to the API as well in this, but the duplication of the code makes sense to remove. I added it into the provides and changed references to where we were using getTrack function or specifically using trackMap for only the purposes of calling getTrack.

subdavis · 2020-11-23T18:53:33Z

+  font-size: 0.8em;
+  max-width: 50%;
+  min-width: 50%;
+}


unused class

subdavis · 2020-11-23T18:53:38Z

+    cursor: pointer;
+    font-weight: bold;
+  }
+}


unused class

subdavis · 2020-11-23T18:54:37Z

+  font-size: 0.9em;
+  padding: 4px 10px;
+  background-color: #272727;
+}


Maybe refactor this to a BorderHighlight.vue with a content slot.

Can probably be a pure functional component.

I added in a PanelSubsection.vue component for structuring the data elements that need to be placed in the Panel and have a scrolling subsection as well. Used for the ConfidenceEditor.vue and both AttributesSubsection.vue components.

I didn't want to go too far into it, but in the future we could add a settings slot and perhaps replace the TypeList and TrackList so that everything has a standard style with a header, expandable settings section, and then a scroll area. It should help with all these vertical subsections that can independently scrolled.

subdavis · 2020-11-23T18:55:05Z

+            <span>Add a new Confidence Pair</span>
+          </v-tooltip>
+        </v-row>
+      </div>


Refactor to component maybe.

I figured that was going to be the eventual plan, didn't know if it should be done here or in a new PR. I can do it here.

Co-authored-by: Brandon Davis <git@subdavis.com>

BryonLewis · 2020-11-24T20:30:52Z

Bug 1
Please let me know if I am misinterpreting this bug.

Datalist is garbage in it's default behavior. Once an item is selected it won't display the rest of the options, that is why I need to reset during onFocus the value to empty/null. I believe it's the same reason it is done on the type selection in the trackItem.vue. It does the same thing as well for MDN's demo implementation of the datalist. It looks like they recognize this and have an example where you can make it work more like it properly should, but it involves creating your own controls for onClick as well as manually manipulating the css style to get to behave properly.

I can go through and using refs and onMount bind the necessary elements and adjust css position to get it to work like the MDN demo. I wanted to get feedback before I go about making my own component.

subdavis · 2020-11-25T13:21:13Z

It does the same thing as well for MDN's demo implementation of the datalist. It looks like they recognize this and have an example where you can make it work more like it properly should, but it involves creating your own controls for onClick as well as manually manipulating the css style to get to behave properly.

Bummer.

I went back and looked again, and this works exactly like the track type dropdown.

I'd say don't worry about it for this PR. The problem is larger in scope than I thought. Maybe we could log an issue and build our own select component sometime in the future to replace all of them (since track type should work like the mozilla demo too IMO).

Thanks for explaining that. It isn't ideal for users, but we should maybe take a minute to think if we really want to maintain/support our own v-select

BryonLewis · 2020-12-01T17:39:03Z

First issue was a bug in the datalist input using a copy of the prop because it needs to be set to nothing when the user clicks on the input so that way the list will properly display. The tempVal wasn't being modified on new props being set so I added a watcher.

The second issue I feel dumb about. It was the classic 0 TrackId and not properly checking that the TrackId !== null instead of just testing to see if the trackId is truthy.

subdavis

Couple more small issues.

Clicking the configure cog only works once. If you click it, then click anywhere outside the dialog, it won't open anymore. Reproduce by just spam clicking the cog. If you use the cancel button inside the dialog, it works properly.

Setting an attribute then immediately clicking the eye makes the attribute disappear on frame 0.
- Steps to reproduce
  - Click a detection.
  - Open the track attribute settings eye, enter some text
  - Click the eye again
  - Text disaappears, but it's still there if you click the eye back on.
Detection attributes seems to have the same problem as above on frame 0: detection attributes disappear in when unused are hidden. This issue is described in a gif in my first review.
"No track selected" text is shown when a track is selected but has no attributes. When no track is actually selected, you see "No track attributes set". These are backward.

BryonLewis · 2020-12-01T20:51:23Z

The cog wheel issue was because I wasn't using a v-model for the dialog and was using a value check. Solved by connecting the click:outside event to the closeEditor function (This was just a dumb oversight on my part).

The issue with the display not showing up after toggling the visibility was caused by utilizing computed properties and having them update without there being any indication for the computed property to update. Would require something like watch deep or using refs and manually updating them using functions after an attribute update is called.

There is another option and that is using the similar style to the TrackListItem.vue and the track.revision property.
https://github.com/VIAME/VIAME-Web/blob/master/client/src/components/TrackItem.vue#L58-L80

It looks like it was a created for a similar reason: Using computed properties derived from a track but the user can edit those tracks and the computed property needs to update properly. In TrackListItem this is when the user clicks on interpolate, delete Feature or add Feature it causes the buttons to refresh by triggering a refresh of the computed function watching the track.revision property. I followed this pattern because the function/watching updating felt messy and why not use the system you already built.

subdavis

Approved! Thanks for humoring all my suggestions.

There's just one other issue I noticed:

Using escape key to close the attribute editor dialog has the same bug as clicking off the dialog: you can't open the dialog again until you open the page.
Also, hitting escape dismisses the dialog and unselects the track, which is kinda jarring because the whole UI changes in an unsuspected way.

I don't know if there's an easy solution to this, or if mousetrap makes it hard. But it might be nice to fix. Could also just log an issue and get to it later.

BryonLewis · 2020-12-02T20:42:57Z

I'll find see what I can find. I imagine it has to do with the v-mousetrap and it's global nature. I know we had that flag used for the global $prompt, may need to make something that prevents it from triggering if dialogs are open. Hopefully without having to directly interact with $prompt This is something we may run into with other areas so I'll look into fixing it.

BryonLewis · 2020-12-03T13:13:54Z

This should prevent the escape from getting to the global mousetrap handler.

subdavis

Went through a more exhaustive test and found one more thing.

You can't edit attributes anymore. If you create one, save it, then edit again and try to change its predefined values, you get a message about attribute already exists.

Also, maybe make the window a tad wider or fix the text wrap?

BryonLewis · 2020-12-03T20:35:49Z

I welcome any change or suggestion to the warning text that is displayed. That text is only displayed when the user is editing an existing Attribute. On brand new attributes that won't be displayed.

I modified the logic so that you can overwrite or change names of the currently selected Attribute. You aren't allowed to duplicate a name that exists when creating a new attribute or when updating a attribute. I.E. if you have [foo, bar, buzz], you can't name a new attribute to any of them, but you are allowed to edit the current attribute to a different name as long as it isn't one of the existing items.

subdavis

🎉 🎉 🎉

BryonLewis · 2020-12-04T19:52:24Z

Sorry, I noticed discrepancies in how the Type datalist in TrackListItem behaves compared to the Attribute version. This will make it so hitting enter after clicking will display the old value before setting it.

I also added an issue #477 about fixing whitespace inputs for both the Attributes datalist and the TrackListItem Type input.

subdavis · 2020-12-04T19:53:36Z

-      (e.target as HTMLInputElement).blur();
+      const target = (e.target as HTMLInputElement);
+      // Datalist needs to reset if we blur on no input
+      if (props.values && props.values.length && target.value === '') {


target.value will be ' ' not '' for datalist index 0. Does this capture that case?

Selecting that manually from the datalist in the dropdown immediately sets it to undefined and bypasses this function. This is for only hitting enter after typing, (or specifically not typing anything). Now hitting enter without adding anything will return it to the default initial value (Same as the type dropdown). Adding in only spaces will remove the value (same as datalist index 0).

With the current setup if you select the 0 index item it immediately executes the change, emitting undefined which will hide the data value if you are editing a single attribute. So the dropdown bypasses the blurType function.

These discrepancies and confusion are why I added that other issue about the behavior of the Type dropdown in TrackListItem.

subdavis

👍

BryonLewis linked an issue Nov 9, 2020 that may be closed by this pull request

[FEATURE] Attributes Panel Updating #413

Closed

BryonLewis force-pushed the client/attribute_ui branch from e75f4fc to 6140e47 Compare November 16, 2020 18:29

BryonLewis added 2 commits November 16, 2020 13:46

Merge branch 'master' into client/attribute_ui

01105c6

Removed old attribute

c5328fd

BryonLewis requested a review from subdavis November 16, 2020 19:12

BryonLewis marked this pull request as ready for review November 16, 2020 19:14

BryonLewis changed the title ~~[WIP] Client/attribute ui~~ Client/attribute ui Nov 16, 2020

subdavis suggested changes Nov 17, 2020

View reviewed changes

subdavis and others added 3 commits November 17, 2020 15:50

Some suggestions (#446)

cb98743

Temporary update to test HTML inputs

34bd34e

Merge branch 'master' into client/attribute_ui

622c47b

Keyboard shortcuts and focusing for HTML Native Attributes

6d2141e

BryonLewis added 6 commits November 20, 2020 08:13

composition API for AttributeInput

dba6e93

Refactor to subsection

06515e5

Merge branch 'master' into client/attribute_ui

ada817f

Need to get scrolling working for this mode

2f9b5b7

Almost got scrolling

5b88e83

Fixing scrolling in attributes and scroll to track settings

986d486

BryonLewis force-pushed the client/attribute_ui branch from 3d84226 to 986d486 Compare November 23, 2020 14:46

subdavis suggested changes Nov 23, 2020

View reviewed changes

Update client/viame-web-common/components/AttributeInput.vue

9fb192e

Co-authored-by: Brandon Davis <git@subdavis.com>

Addressing comments

6bdc4da

BryonLewis linked an issue Nov 25, 2020 that may be closed by this pull request

Attributes Text data with spaces not stored properly #134

Closed

Merge branch 'master' into client/attribute_ui

d67b8ac

subdavis suggested changes Dec 1, 2020

View reviewed changes

BryonLewis and others added 2 commits December 1, 2020 15:41

Adressing more of my mistakes

4e5cc97

Merge branch 'master' into client/attribute_ui

02e72c7

Merge branch 'master' into client/attribute_ui

e34709a

subdavis previously approved these changes Dec 2, 2020

View reviewed changes

escape focusing

34e4770

BryonLewis dismissed subdavis’s stale review via 34e4770 December 3, 2020 13:12

BryonLewis requested a review from subdavis December 3, 2020 14:12

subdavis suggested changes Dec 3, 2020

View reviewed changes

BryonLewis and others added 2 commits December 3, 2020 14:30

Merge branch 'master' into client/attribute_ui

a7281be

Edit Attributes Updates

af34b2f

BryonLewis and others added 3 commits December 3, 2020 15:37

mend

39c9729

Small fix (#476)

accbd86

Merge branch 'master' into client/attribute_ui

7f874b7

subdavis previously approved these changes Dec 4, 2020

View reviewed changes

datalist updates

7814149

BryonLewis dismissed subdavis’s stale review via 7814149 December 4, 2020 19:50

BryonLewis requested a review from subdavis December 4, 2020 19:52

subdavis reviewed Dec 4, 2020

View reviewed changes

fixes clicking on open datalist attributes

4cdf25e

subdavis approved these changes Dec 4, 2020

View reviewed changes

BryonLewis merged commit cb4f7d7 into master Dec 4, 2020

BryonLewis deleted the client/attribute_ui branch December 4, 2020 21:23

Conversation

BryonLewis commented Nov 6, 2020 • 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

Issues

UI Feedback

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis commented Nov 19, 2020

Uh oh!

subdavis commented Nov 19, 2020

Uh oh!

BryonLewis commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryonLewis commented Nov 23, 2020

Uh oh!

subdavis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Bug1

Bug2

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!

BryonLewis Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

BryonLewis commented Nov 24, 2020

Uh oh!

subdavis commented Nov 25, 2020

Uh oh!

BryonLewis commented Dec 1, 2020

Uh oh!

subdavis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis commented Dec 1, 2020

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

BryonLewis commented Dec 2, 2020

Uh oh!

BryonLewis commented Dec 3, 2020

Uh oh!

BryonLewis commented Nov 6, 2020 •

edited

Loading

BryonLewis commented Nov 19, 2020 •

edited

Loading

subdavis left a comment •

edited

Loading

BryonLewis Nov 23, 2020 •

edited

Loading

BryonLewis Nov 24, 2020 •

edited

Loading

subdavis left a comment •

edited

Loading

subdavis left a comment •

edited

Loading

BryonLewis commented Dec 3, 2020 •

edited

Loading