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

Add groups #2826

Merged
merged 22 commits into from
Jun 6, 2021
Merged

Add groups #2826

merged 22 commits into from
Jun 6, 2021

Conversation

bewee
Copy link
Member

@bewee bewee commented May 16, 2021

This proposes adding directories as a means of grouping things. Directories get placed below things. You can drag a thing into a directory or remove it from its current directory. You can add (through the "+" screen) and delete directories and change the order among them. You can also open or close each directory, and which directories are open gets cached on the current device.

image

On the way, I also reworked the layout indexing mechanism so that things no longer "flicker" after being dropped (this was especially annoying when e.g. dragging the last thing to the first position, or when your Internet connection was slow, or even both).

I know that translations as well as a feature for renaming directories are still missing, and I would add these in case you like the general idea of having directories. Some stuff may still be a little rough or maybe sort of "not-the-way-the-gateway-does-it". In this case particularly, but concerning everything else as well of course, I would really appreciate any feedback :)

@kgiori
Copy link
Contributor

kgiori commented May 16, 2021

Pretty cool! I would definitely benefit from this approach. My things page has gotten pretty crowded (52 things), and several of the things that I don't interact with (scenes, notifiers, schedulers, and more) would be nice to drop into a low-level directory.

@tim-hellhake
Copy link
Member

@bewee
I just tested it.
That's awesome!
You should be able to resolve the formatting issues by running npm run prettier.

in case you like the general idea of having directories

Absolutely!

@benfrancis
Copy link
Member

benfrancis commented May 18, 2021

Hi @bewee,

Thank you very much for this contribution and for the significant effort you've put into implementing it.

The UI is very cool. I'd like to follow-up with some visual CSS tweaks, but that's a minor issue.

You may have noticed that there has been some previous discussion about grouping things in #1191, including some alternative approaches.

My main feedback is about the term "directory", and the fact that (if I have understood correctly) a Thing can only belong to a single directory. This is in contrast with other proposals like "groups" or "tags" which may allow things to belong to multiple groups.

The Web Thing API exposed by the gateway is currently in the process of standardisation at the W3C (the REST API as a "Core Profile" and the WebSocket API as the "Web Thing Protocol"). In the W3C specifications the term "directory" will soon have a special meaning, with the definition of a Directory Service API for exposing a collection of Things (like your /directories API endpoint) and a Thing Description for describing directories (like your directory descriptions).

The (work in process) W3C Directory Service API doesn't have exactly the same purpose as the directories feature you have implemented here, as it's not really designed for a gateway to have multiple directories, but rather to act as a single directory which can be filtered/queried using JSONPath/XPath/SPARQL. However, I have recently proposed an alternative approach which could allow for multiple directories. I suppose you could theoretically use the W3C's current approach for multiple directories by providing a Thing Description and API endpoint for each, but it's not really designed to be used that way.

My concern with the API implementation in this PR is that if and when we come to implement the W3C Directory Service API, this is going to get confusing. Therefore I can see two possible paths forward for this feature:

  1. Converge this /directories API with the W3C Directory Service API and track that standard as it evolves (note that there is some discussion about the URL structure of that API being normative, though I'm arguing for more flexibility, and the API is very much in flux at the moment). This could either work by having the gateway expose multiple directories, or by adding some kind of group/tag member to Thing Descriptions of Things as a semantic annotation by which a single directory of things could be filtered.
  2. Rename this feature to something else like "groups" (potentially also allowing Things to belong to multiple groups) and make it an internal-only feature of the gateway which isn't part of the standard at all, using an API endpoint separate to /things (as you currently are), or vendor-prefixed semantic annotations to Thing Descriptions to denote which groups they belong to, e.g.
{
  ...
  wt:groups: ['kitchen', 'downstairs', 'indoors']
}

Number 1 would be cool, but number 2 would likely be easier and quicker to land.

@bewee
Copy link
Member Author

bewee commented May 18, 2021

Hi @benfrancis,

I'd like to follow-up with some visual CSS tweaks, but that's a minor issue.

Alright, if you describe those I could try fixing them. But I guess it may be simpler if you just do these changes by yourself?😅

You may have noticed that there has been some previous discussion

Oops, actually I didn't. I think I only searched the issues for the keywords "directory", "folder" and "category".
I do like the idea of putting one thing in multiple groups. It also shouldn't be so much of a problem to store multiple directory ids per thing instead of just one (as of now). However, I in the current approach it may be be quite difficult to maintain multiple layout indices per thing, one for each directory? Also, I think it would then no longer make sense to "move" a thing into a group (so that it is no longer listed under the "things without directory" section)? I think that would be a bit of a loss, since directories as in this suggestion allow you to move all your dummy things which only act as a time trigger for the rules engine or something into a trash folder which you then move all the way down to the bottom in order to forget it.

From my personal understanding, "groups" should probably rather group similar devices in order to control them together. #1191 (comment) mentions doing this using an add-on, which I think is a feasible idea (since with this suggestion, you could now move all the grouped things into one directory that you almost never have to touch again) and only keep the group device up on top. Thoughts?

Converge this /directories API with the W3C Directory Service API and track that standard as it evolves

I do like this idea, however I have no clue how feasible it is. I will have a look at the spec you linked, but as you already say that it's not really meant to be used that way, I have the feeling that it may be a bad idea to force this suggestion into the W3C API?

Rename this feature to something else like "groups"

Just want to also propose renaming it to "categories" (that's the second choice I had in mind when building this, but unfortunately I decided in favour of directories🙈)

@bewee
Copy link
Member Author

bewee commented May 18, 2021

Oh, and one more thing that I think would be pretty cool is adding directories (or categories or groups or whatsoever) to the "Generate local authorization" screen + permission system. But I did not yet have a look at how complex this would become

@benfrancis
Copy link
Member

@bewee wrote:

if you describe those I could try fixing them. But I guess it may be simpler if you just do these changes by yourself?

That's fine, I don't want to hold up your PR on visual design nits.

I do like the idea of putting one thing in multiple groups. It also shouldn't be so much of a problem to store multiple directory ids per thing instead of just one (as of now). However, I in the current approach it may be be quite difficult to maintain multiple layout indices per thing, one for each directory? Also, I think it would then no longer make sense to "move" a thing into a group (so that it is no longer listed under the "things without directory" section)?

I agree, having things belonging to multiple groups doesn't really fit your current UI and API design. In some ways it would be a shame to lose that, because I like how simple the UI is.

I think that would be a bit of a loss, since directories as in this suggestion allow you to move all your dummy things which only act as a time trigger for the rules engine or something into a trash folder which you then move all the way down to the bottom in order to forget it.

This smells like a workaround for some unfulfilled requirements we should be solving in a different way. If we design the system well, there shouldn't be a need for "dummy things" that you need to hide from view. It would be helpful if you could (in a separate issue) explain the reasons that you currently have dummy things, and maybe we can find better solutions to those problems.

From my personal understanding, "groups" should probably rather group similar devices in order to control them together. #1191 (comment) mentions doing this using an add-on, which I think is a feasible idea (since with this suggestion, you could now move all the grouped things into one directory that you almost never have to touch again) and only keep the group device up on top. Thoughts?

For the same reasons, I don't really think this is a great solution. A dummy thing acting as a placeholder for a group of other things is just another workaround.

I have the feeling that it may be a bad idea to force this suggestion into the W3C API?

I agree. This isn't really an intended use case for that API and those discussions are already complicated enough!

Just want to also propose renaming it to "categories" (that's the second choice I had in mind when building this, but unfortunately I decided in favour of directoriessee_no_evil)

I agree the name "directories" is a better name than "categories", since they can be used for lots of different use cases like grouping devices by room, device type, owner etc. It just conflicts at the API level with the concept of the gateway as one big directory of devices as being defined at the W3C. The conclusion from #1191 seemed to be that "groups" was a good generic term which covers all of these use cases.

If you wanted to experiment with an alternative implementation that allows devices to belong to multiple groups then that would be cool, but I'd also be fine with landing this feature with the limitation of things only being allowed to belong to one group in order to keep the UI simple. If someone then wants to expand that to allow more complex groupings then they can either:

  1. Figure out how to extend your UI and API implementation to allow things to belong to multiple groups
  2. Implement a separate "tags" feature which allows devices to have multiple tags assigned to them (likely exposed at the API level as semantic annotations in their Thing Description)

What do you think?

@bewee
Copy link
Member Author

bewee commented May 20, 2021

Hi @benfrancis,

The conclusion from #1191 seemed to be that "groups" was a good generic term which covers all of these use cases.

I have absolutely no problem with the name "groups", just wanted to bring up another alternative :) I can do the renaming to "groups" soon and update this PR (from now on, I will just call this feature "groups" and reference to the feature described in #1191 as "tags")

It would be helpful if you could (in a separate issue) explain the reasons that you currently have dummy things, and maybe we can find better solutions to those problems.

See #2828

A dummy thing acting as a placeholder for a group of other things is just another workaround.

I somewhat disagree here.
My understanding of #1191 is that the tags feature described there allows for controlling multiple (most likely similar) devices at once, thus acting as a sort of forwarder to many devices. For instance, I would want to turn on all the lights in my bedroom at once. However, I also still want to be able to turn on each light on its own.
So, what we need is some sort of entity to control many things at once. This entity could be a core gateway feature, e.g. adding another "tags" view which lists all tags and through which one can control all devices summarized under the same tag, add new or edit existing tags and tag / untag things. However, I feel like these sort of tags do have quite a bit in common with the concept of already existing things - just instead of controlling one physical device, they forward to a number of those. I agree that such a feature is not really the responsibility of an adapter. However, it is possible, and implementing such a feature through an adapter would be strikingly simple and functional.

My private workaround right now is to have a virtual thing "Main Light Controller" which (programmed via Node-RED add-on, but similar things can be done using the rules engine) forwards to the four lights in my room. I find it extremely convenient to be able to control all my stuff at once (what i mostly do), but to still have the "original things" laying around somewhere down in a folder when I need them. And tbh I really like the simplicity of this approach, interacting with multiple devices through one thing that lays directly on my gateway's home page (and not in one more submenu or something). Setting this up, as well as adding another light to the main controller, is a bit of a pain. And I believe that an integrated tags feature, just as well as an add-on adapter, would be an extremely great way to make such a setup simpler. I would prefer the build-in variant because I believe that it would be way more user-friendly (with an adapter add-on, you most likely would have to deal with inserting some stuff into the add-on config). What I would mainly like to plead for is implementing tags to be part of the things view, to look like (or really similar to) a thing and to be controlled like a thing (in short, no new UI one has to get used to).

So to sum things up and answer your question: I think that groups (a means of bringing some structure into the big bunch of things) are a fundamentally different feature than tags (a means of controlling similar devices all at once). So I would prefer to land this feature the way it is, with one thing only being allowed in at most one group at once. But when I find some time, I would love to have another look at the tags feature as I just tried to describe it :)

@bewee bewee changed the title Add directories Add groups May 21, 2021
@flatsiedatsie
Copy link
Contributor

My 2 cents:

It seems Bewee just wanted a quick way to organise the things overview page.

The gateway-wide grouping feature (#1191), which could be foundational for all kinds of functionaity, seems to be me to be a different kettle of fish. A much bigger kettle.

With that in mind, that big future functionality might need to be called 'groups'. So why not call this PR's structure something that reflects the "a thing can only placed in one" nature. If it's just a small upgrade of the things overview page, then directory or "folder" makes sense to me.

@bewee
Copy link
Member Author

bewee commented May 25, 2021

Is there anything else I can do for now?

Copy link
Member

@tim-hellhake tim-hellhake left a comment

Choose a reason for hiding this comment

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

Sorry, I was a bit busy.
Looks good to me 😄

@tim-hellhake
Copy link
Member

Looks like you need to run prettier 😅

@bewee
Copy link
Member Author

bewee commented Jun 3, 2021

So, what do we want to do now concerning naming?

@tim-hellhake
Copy link
Member

tim-hellhake commented Jun 3, 2021

Looks like I need to approve every build o.O
Now stylelint is failing :(

So, what do we want to do now concerning naming?

I'm fine with groups.

@bewee
Copy link
Member Author

bewee commented Jun 4, 2021

On my local machine, there are 15 more failing tests from 3 test suites, most of them with error TypeError: Cannot read property 'url' of null during await browser.url('/'). I have absolutely no idea where this comes from or how to fix it :/

Also, should I write some additional tests for the groups feature? If so, how extensive should the tests be?

@bewee
Copy link
Member Author

bewee commented Jun 6, 2021

Could we give the build another try? The mentioned failing tests may have only been a problem with my machine...

@tim-hellhake
Copy link
Member

Could we give the build another try? The mentioned failing tests may have only been a problem with my machine...

Sure^^

@codecov-commenter
Copy link

Codecov Report

Merging #2826 (8e7596e) into master (772e98b) will decrease coverage by 1.51%.
The diff coverage is 31.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2826      +/-   ##
==========================================
- Coverage   66.52%   65.01%   -1.52%     
==========================================
  Files         121      124       +3     
  Lines        7615     7938     +323     
  Branches     1289     1314      +25     
==========================================
+ Hits         5066     5161      +95     
- Misses       2506     2735     +229     
+ Partials       43       42       -1     
Impacted Files Coverage Δ
src/models/group.ts 14.70% <14.70%> (ø)
src/db.ts 68.96% <22.22%> (-8.74%) ⬇️
src/models/groups.ts 26.92% <26.92%> (ø)
src/controllers/groups_controller.ts 27.00% <27.00%> (ø)
src/models/thing.ts 68.09% <40.00%> (-0.69%) ⬇️
src/models/things.ts 75.46% <52.63%> (-6.50%) ⬇️
src/controllers/things_controller.ts 80.19% <80.00%> (-0.21%) ⬇️
src/constants.ts 100.00% <100.00%> (ø)
src/router.ts 98.94% <100.00%> (+0.02%) ⬆️
static/js/schema-form/schema-utils.ts 83.33% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 772e98b...8e7596e. Read the comment docs.

@tim-hellhake
Copy link
Member

tim-hellhake commented Jun 6, 2021

On my local machine, there are 15 more failing tests from 3 test suites, most of them with error TypeError: Cannot read property 'url' of null during await browser.url('/'). I have absolutely no idea where this comes from or how to fix it :/

The integration tests often fail without any obvious reason.

Also, should I write some additional tests for the groups feature? If so, how extensive should the tests be?

Normally I would say absolutely but I don't want you to suffer from integration test nightmares.
Given the fact that one of the jobs was merciful enough to let the integration test pass, I would rather merge it now.

@tim-hellhake tim-hellhake merged commit 9ae796e into WebThingsIO:master Jun 6, 2021
@tim-hellhake
Copy link
Member

I'd wish I could simply release it now.
Maybe we should do something like nightly builds for the docker images.

@bewee bewee deleted the bewee/patch-1 branch June 6, 2021 22:49
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.

6 participants