-
-
Notifications
You must be signed in to change notification settings - Fork 241
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
New option to display equipment as accordion in Locations cards #1307
Conversation
… the possibility to promote the main item of the equipment (widgetOrder == 0) as the accordion opening item, while keeping the functionality of the control. Signed-off-by: Gautier Taravella <tarag@mailbox.org>
Job #387: Bundle Size — 10.7MB (~+0.01%).Changed metrics (2/10)
Changed assets by type (2/7)
|
All the changes are linked to new options (it seems people don't like unexpected changes 😉) Still a little bit a work in progress. Code-wise, I know there are some improvements to be made, specially in homecard-mixins.js as there is quite a bit of duplicate code. But for this I would like to propose to remove the Equipment | Property top level distinction in Location/Equipment cards. I really don’t see what is the purpose of this or how to explain a user what he will find behind each category. I don’t use Properties at to level in Locations but looking at the openhab demo I really wonder what is the purpose of this distinction in Locations card (I don’t question the purpose of properties in the model of course). @ghys , do you think this distinction is really useful in this part of the UI? I also would like to use equipment accordion opening list items with custom list widgets (like the central light controller in screen recording) but this does not work. I need to figure out a way to pass the accordion slots context along the way for custom widget. Do you have an idea? By the way some corrections were performed so that sub-equipments are not forgotten in these pages. |
Thank you very much for this @tarag because it is pretty much what I had in mind for quite some time - I even made some experiments a year ago or so but didn't pursue them further because I had some problems with accordions as you found out yourself. I'm giving it a quick try and it works pretty well - with a few rendering issues but nothing major, we can address them as we go along - there is something that I can't figure out though (I only looked at the code in passing yet): The accordions in the location cards seem to work when enabled but not the equipment cards: So I'm not sure if i'm missing something. Also maybe it would be nice to repeat the options on the cards to override those set at the tab level Great work overall though, I think we can push this forward.
It was implemented only because the model allow this (property -hasLocation-> location) and it's useful in some cases, like for NLP (HABot & co.), when you ask "what's the temperature in the living room?" the Measurement>Temperature property directly under the room location is supposed to take precedence over any such property part of equipment in that room. |
Btw another idea that I had, off-topic for this PR but still, is to have an "advanced" flag for items defined in metadata, maybe in the
Advanced items wouldn't be shown by default unless you check a box (similarly to configuration properties). |
Glad you like it, it did not really experiment it other than testing but I really think it’s an improvement.
The PR doesn’t eventually have lots of code lines for this mechanism but it took quite a few hours of trial and error, head scratching and deciphering f7 code to reach this state. F7 does lots of behind the scene work with events that would prevent reaching the expected result. It does rely on what appears to be some internal API but I don’t think it will change. Maybe we could ask for an evolution of f7 to support this without fighting against it….
This is not yet implemented. It will rely on the same mixin but there are a few labelling changes to make in addition. I wanted to show the principle first before continuing.
The latter. I perfectly understand the reason to have
I am also thinking a bit about how to handle this. The current solution I use with other account/roles and use of With this PR I it’s less useful though because these items would not be displayed when the accordion is closed. I also thought defining a sub-equipment labelled « Advanced » that would gather all these items but it is somehow an abuse of the model and a dedicated metadata flag and dedicated UI control would definitely be better. |
I was also thinking about this capability, maybe not necessarily providing the UI for it (just allowing it if defined in YAML).
No problem for the options names, I’ll use the full « equipment » word. |
Removed duplicate filtering and sorting of model items (2-3 fold execution time reduction). Factorized some equipment display code. Renamed equipment options. Implementation of accordion style for equipment in Equipment page. Slots assigned to the custom list item widget are now copied to base widget so that accordion chevron and is displayed and open even works for custom widgets used as equipment accordion open button. Detection of loop in semantic model to prevent stack overflow error and indicate correction path. Signed-off-by: Gautier Taravella <tarag@mailbox.org>
Framework7 is a one-man show, we are already one major version behind (v5, migrating to v6 implies switching to Vue 3 and would involve a lot of work), and the author only seems to support the latest version (apart from critical bugs).
Not so surprising, the demo items were there for a long time (maybe since 1.x.) and was directly taken over to 3.x, and some items (like the temperature in a room) were directly attached to the room's Group, the general principle of having Equipment groups didn't exist at the time. Still it makes sense to attach properties or points to Locations - I've mentioned a couple of times that you can modify the group membership of a Measurement>Temperature or Setpoint>Temperature Point to be both under an Equipment group (measurement or setpoint as reported by a thermostat) and a Location group (what is considered to be the temperature measurement/setpoint for the whole room) - the semantic model allows it and it also makes sense semantically. Also I'm not sure that all Properties under a Location should also be in an Equipment under the same Location.
I'm not so sure, accordion or no, it would be nice to have only the most important items displayed (as an end user of those cards, you care mostly about a few items and the rest would be only on a needed basis, when you check the "Show Advanced" box). Having this flag set automatically when you convert a thing's channels to an Equipment (depending on the advanced status on the channel) would be nice too. |
I fully agree, but one has to be careful so that it makes sense to the user. I believe that a non-Property Point should go along with the Property in the Location cards context don’t you think? They are just a less precise kind of Property. (This is what I did in today’s commit)
I did not know that. I’ll have to check if I did not make assumptions in the code that could cause issue in this regard.
I deal mostly with KNX installations where almost all the equipment is in the electrical cabinet in the basement or elsewhere. I completely disregard this and it does not appear anywhere in the UI to the user. I think this is the distinction between Things (devices) and Items (function). Equipment are group of items and it make sense for the end-user to place them in the room where the function is provided (if the function is not shared among a single room, then I’d place it in the upper-level Location that would make sense). I do have a few Equipment in the room where the device is physically installed but this is for maintenance Points/equipment shared among several user function-equipment available in the corresponding room (for example a heating boiler). I find this maps quite well to voice assistants also. |
@ghys, did you have a look at the latest commit ? Could you tell me what would be needed to go forward (you said there were some cosmetic issues) ? Related to our discussion, Points belonging to the model directly under a Location are now displayed under the 'Properties' sub-part of location cards (they would not show-up anywhere before...). So even if I don't necessarily plan to use them I thinks that's an improvement. |
I admit I didn't, but I thought it was still a work in progress? |
There is always room for improvement but I believe it’s quite consistent right now. In addition, merging would allow me to tackle the configuration of the labelling that was raised in the community discussions. |
Ok, but options in the equipment list that aren't implemented and do nothing should be removed first then (or commented out or hidden):
|
This should work for equipment (it works for me). Are you sure you fetched the latest commit? |
bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Gautier Taravella <tarag@mailbox.org>
@ghys, sorry for harassing you 😉 Could you tell me what should be improved before a merge is possible ? |
@tarag thanks for the update, I just had a look (I have low bandwidth for OH these days) and had to set up my dev environment again. So it's probably linked to the changes wrt. the card model building in homecards-mixin.js although I have to diagnose why (there's nothing in the console). In any case since there's clearly potential for regressions it needs to be tested extensively (since there are still no unit/e2e tests although the infrastructure is there). |
…ation, equipment, properties). Signed-off-by: Gautier Taravella <tarag@mailbox.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, it seems to work better at least in my case!
I left some comments below:
bundles/org.openhab.ui/web/src/assets/definitions/widgets/home/index.js
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/assets/definitions/widgets/home/index.js
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/assets/definitions/widgets/home/index.js
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/cards/equipment-card.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/components/widgets/standard/list/default-list-item.js
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/pages/settings/pages/home/home-edit.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing wrt. additional styles in oh-list-item
bundles/org.openhab.ui/web/src/components/widgets/standard/list/oh-list-item.vue
Outdated
Show resolved
Hide resolved
Signed-off-by: Gautier Taravella <tarag@mailbox.org>
Signed-off-by: Gautier Taravella <tarag@mailbox.org>
Not sure what this not successful check is about. I think everything should be all good now. |
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think we can at last ship this change. The code is clear enough and I believe there aren't any obvious regressions esp. in the behavior of system widgets, but if we missed something it can be dealt with.
To avoid another back-and-forth I finished cleaning up the styles in oh-list-item (putting everything under the dedicated class), and tweaked the loading indicator in the home page designer to avoid flickering, a minor change.
The build failure is not related, the version of HABot's code when you forked the main branch doesn't build with the latest core, it's been fixed since.
Many thanks for all your work and your patience in the last few months!
It would be great if you could send me the result of the REST call: '/rest/items?metadata=semantics,listWidget,widgetOrder' of your installation, to be performed via the 'Development' > 'API Explorer' page. |
I just restarted with .M4 :
to check chrome console's : there is nothing at all displayed in console. I don't have any errors in openhab.log, by the way (outside a few WARN like some (for now) empty .things or .rule files) or a few weird things (like an error "cannot instale bindings" (whereas they are already there ...), but not related (and same ones with .M3). |
If the location tiles where displayed in M3 and not in M4, it could very well be linked to this PR. What would really be helpful is the result of the REST API call of items, including the metadata. You can send it via email, I don't think there is anything too much sensitive (it's the current state of your installation). |
Yes, sure. |
After a quick look to the files you sent me, it seems that the missing location cards between M3 and M4 have no equipment/points, and are therefore not displayed. Looking at your items, it appears that some equipment have 2 parents: one container Equipment, and one Location. This is for example the case of the following Equipment:
Therefore it is displayed in the Equipment tab but not in the Location one, because somehow Equipment belonging has precedence over Location belonging. This PR makes the assumptions that the model to be consistent with the definitions given in the docs, and more precisely with these sentences: A Point can only be the direct member of zero or one Location, or the direct member of zero or one Equipment. Put another way, a Point can only be the direct member of one Group that has a Location or Equipment tag. @ghys : Are these descriptions no longer accurate ? Not having a single parent will lead to all sorts of difficulties one all my recent PRs... For example, what parent to display in the path of the item, the Equipment or the Location ? |
This is false! You can only have at most one parent for a given relation type (isPartOf, isPointOf, or hasLocation, not considering hasPoint because it's not reliable) but it is perfectly valid to have multiple relations when they're not incompatible.
In the Location card, I think the equipment path (as you already know the location), and in the Equipment card, the location path. |
…b#1307. Changes the parsing of the model so that Equipment or Points that are respectively sub-Equipment or Part Of an Equipment while having a Location parent can be displayed in the relevant Location tab card. Signed-off-by: Gautier Taravella <tarag@mailbox.org>
This change, and the others that might be link are not trivial so at least I made a quick fix that restores the way Equipment and Points were previously selected for Location cards. At least it will prevent users from seeing disappearing items in their Location cards, while a more in-depth solution is found. A new PR has been made: #1385 |
…#1385) Changes the parsing of the model so that Equipment or Points that are respectively sub-Equipment or Part Of an Equipment while having a Location parent can be displayed in the relevant Location tab card. Signed-off-by: Gautier Taravella <tarag@mailbox.org>
Could you explain the model tagging of these items? I suppose Hayward OmniLogix is an Equipment and its children are also Equipment ? Are there any Points below OmniLogix ? Please show me a screenshot of the Model page with this group expanded. |
I did not check again as I'm running everywhere on the milestone 3.3, but my understanding from version 3.2 code is the following. When displayed in location cards, an Equipment would be displayed as a header (divider) and all the points of that equipment would be displayed below, but sub-Equipments would not be displayed at all. To get sub-equipment displayed then the parent equipment should not have had any point children, and in that case the Equipment would be displayed as a list item leading to the group contents (not only the model contents). This is probably what you were used-to from the Hayward OmniLogix equipment since it does not have Point children. I thought this behaviour was a bug, or at least something only partially implemented so in this PR, I made sure that all the model children of an equipment are displayed in the location cards, wether they are points or sub-equipment. I agree that it changes what you were used to but is this really an issue @matchews? No you get the contents of the OmniLogix equipment with one less click. On the other hand, if you'd prefer not to have everything displayed at the first level then maybe switching to accordion is a good solution for you ? Then we can see how to get the temperature displayed where you'd like. What do you think about this unexpected change @ghys? Was 3.2 design about sub-equipment on purpose. I don't recall that from our discussion. |
I think it works even with Points. I have 2 cases of a location>equipment>sub-equipment>points tree they're still working as expected both with the accordion feature disabled and enabled. @matchews if I'm not mistaken, your temperature point above is a member of 2 equipment groups at the same time ("Body of Water" and "Heater") and that is not supported by the semantic model - perhaps that's what causing the problems. |
I've removed the duplicate group memberships on any items with more than one parent with no change in behavior. The new accordion feature is nice, but still does not show the "Item to Display" as setup in the default list item meta data. With the new Equipment List/Display sub-equipment levels feature set to either option selected, or no option selected, I no longer see the "Item to display". Thanks again for the help. |
Ah OK, I just got what you meant by "item to display", you redefined the 'item' property of the group to display the temperature. As a workaround you might want to set one of the |
@ghys, maybe we can revert #1398 and do the simple and evident correction you suggested in the comments there instead. There might be other people using itemToDisplay in groups, I had not thought of this setting possibility. @matchews, if you use accordion presentation and also 'promote' the temperature item by setting the widgetOrder to 0 and selecting the right option, you will get the temperature displayed as the header of your heater group. |
I just issued the corresponding PR #1435. |
It does allow to control item, and that's the main reason I coded it. I find it very useful to get the most used functionality promoted one level higher. Unfortunately, a very recent PR breaks the event handling in the accordion item and therefore breaks this functionality. I hope the correction is merged for 3.3...
The |
This PR provides a new option to display equipment as accordion in Locations cards, also giving the possibility to promote the main item of the equipment (widgetOrder == 0) as the accordion opening item (or equipment with a single point), while keeping the functionality of the control in the list item:
This is an improvement when having very long list of equipment in one location, as it give a fast access to each point, while not having to scroll a lot and be overwhelmed by the quantity of items. A good selection of the main item for each equipment and selection of the 'promote' option further increases speed of use.