Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if item exists when creating new, fixes #573 #1000

Merged
merged 1 commit into from May 11, 2021
Merged

Conversation

hubsif
Copy link
Contributor

@hubsif hubsif commented Apr 15, 2021

This addresses #573. It adds an existence check to item-form.

Since the UI would load the items from the server multiple times and also each time the radio button is toggled between "new" and "existing", I modified item-form and item-picker to accept already loaded items as property. Items are now loaded in link-add (if required) and passed on to those components.

Originally I wanted to start migrating this to Vuex. But during planning I realized that this would become too big a change, since I think when doing this the whole REST API should be wrapped in a cache (e.g. vuex-rest-api-cache), so unchanged stuff doesn't always get loaded from the server. Since this would have had major impact in API communications and required a lot of time for testing, I dropped it.

@hubsif hubsif requested a review from a team as a code owner April 15, 2021 18:09
@hubsif
Copy link
Contributor Author

hubsif commented Apr 15, 2021

Hooray, I caught the 1000! 😜

@relativeci
Copy link

relativeci bot commented Apr 15, 2021

Job #114: Bundle Size — 10.44MB (+0.02%).

ca1684b vs c98ce20

Changed metrics (2/8)
Metric Current Baseline
Initial JS 1.6MB(+0.02%) 1.6MB
Cache Invalidation 21.15% 15.48%
Changed assets by type (1/7)
            Current     Baseline
JS 8.14MB (+0.03%) 8.14MB

View Job #114 report on app.relative-ci.com

@hubsif
Copy link
Contributor Author

hubsif commented Apr 16, 2021

Just uploaded a slight adjustment that fixes the Vue warning to not alter props.

@ghys
Copy link
Member

ghys commented Apr 18, 2021

Not a bad idea, but I think it would cause all items to be loaded again when the item form is displayed in the Model page.

model.vue => item-details.vue => item-form.vue

<div class="padding-top" v-if="editMode">
<item-form :item="editedItem" :hide-type="true" :force-semantics="forceSemantics" />
</div>
<div class="padding-top" v-else-if="createMode">
<item-form :item="editedItem" :enable-name="true" :force-semantics="forceSemantics" />
</div>

The model component already has a flat list of items at hand so it could pass it to item-details as a prop (currently it has only a model prop which is the hierarchy as displayed in the tree view, not a flat list), which would in turn pass it to item-form.

Storing the items in Vuex is naturally the best way to go, but the challenge would become to detect/invalidate the cache when that list has changed - apart from keeping an EventSource open, which I don't think is reasonable, I have no good solution for this. That's why for instance the item picker fetches the list of items every time - they might have changed at any time, not only from the current UI tab. See also #793.

@hubsif
Copy link
Contributor Author

hubsif commented Apr 18, 2021

Not a bad idea, but I think it would cause all items to be loaded again when the item form is displayed in the Model page.

I see.

Storing the items in Vuex is naturally the best way to go

So, does that mean passing the items as props would be ok (for now)? (since Vuex and caching would be more of a big, "global" change)

@ghys
Copy link
Member

ghys commented Apr 18, 2021

So, does that mean passing the items as props would be ok (for now)? (since Vuex and caching would be more of a big, "global" change)

Yes, sorry. I was talking "in the long run". Even if the info is rather volatile because it has to be checked regularly for changes, for instance the Model page could tell the Vuex store's items list to update and the children components would reuse it. There could even be some logic on how to update, that would avoid fetching the entire response if it's not necessary, like a ETag/If-None-Match or Last-Modified/If-Modified-Since headers combination - provided support is added in the backend (https://www.logicbig.com/tutorials/java-ee-tutorial/jax-rs/request-context.html).

@hubsif
Copy link
Contributor Author

hubsif commented Apr 21, 2021

Not a bad idea, but I think it would cause all items to be loaded again when the item form is displayed in the Model page.
model.vue => item-details.vue => item-form.vue

I just committed an update.

@hubsif
Copy link
Contributor Author

hubsif commented Apr 23, 2021

I just discovered that this actually also fixes #906.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Thanks, mostly LGTM, have a few final remarks.
Also it would be good to expand the cautious "sharing" of these lists of reusable items in the Vuex store as discussed, since passing such data as props down the component tree is not ideal and not a Vue best practice. But for now, it's fine!

@hubsif
Copy link
Contributor Author

hubsif commented May 10, 2021

Ok, so I updated this and checked it more thoroughly 😅. Changes:

  • item-form does not load items anymore. If enableName is true (which could be renamed to sth. like editMode 😉), it expects the items to be provided as prop
  • all usages of item-form with enableName set to true now provide items
  • for above channel-list now accepts items as a prop, required in multipleLinksMode for item-form
  • add-from-thing now loads all items, to provide them to item-form and channel-list (so items get only loaded once)
  • item-edit now loads all items, to provide them to item-form and item-picker

As discussed before, having the items handled and cached in a Vuex-store would be much better. I'm planning on doing some tests and maybe open a community forum thread for discussions regarding this.
Until then, I think the work provided here is ok for now.

@ghys ghys merged commit 65df3b3 into openhab:main May 11, 2021
@hubsif hubsif deleted the fix_573 branch May 11, 2021 19:02
@ghys ghys added this to the 3.1 milestone May 30, 2021
@ghys ghys added main ui Main UI enhancement New feature or request labels May 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants