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

Model editor: Drag and drop #2970

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Jan 3, 2025

Closes #2728
Closes #969

Allow drag and drop in the model view:

  1. Drag and drop inside the semantic model
  2. Move items into the semantic model
  3. Move between non-semantic groups (and duplicate)
  4. Ask for creation of location, equipment or point if item does not have a semantic class yet when moving into the semantic model

This PR requires further testing, but I do want to already provide it for others to start evaluating, as there is a fair amount of change. I probably have missed some boundary cases of changes in the model that are or are not possible.

Edit: I have tested this on different devices, and from my perspective, it works well. But again, I may have missed something. Therefore I would appreciate others to test.
I have put extensive debug logging into the code to be able to debug potential issues.

Here are a few gifs of what it looks like:

ModelMove1

ModelMove2

@mherwege mherwege changed the title [mainUI] Model drag and drop Model editor: Drag and drop Jan 3, 2025
@digitaldan
Copy link
Contributor

That's super cool.

Copy link

relativeci bot commented Jan 3, 2025

#2867 Bundle Size — 11.02MiB (+0.25%).

0f229a1(current) vs cceb442 main#2835(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
#2867
     Baseline
#2835
Regression  Initial JS 1.9MiB(+0.05%) 1.9MiB
No change  Initial CSS 577.29KiB 577.29KiB
Change  Cache Invalidation 20.06% 17.32%
No change  Chunks 227 227
No change  Assets 250 250
Change  Modules 2953(+0.03%) 2952
No change  Duplicate Modules 154 154
No change  Duplicate Code 1.8% 1.8%
No change  Packages 98 98
No change  Duplicate Packages 2 2
Bundle size by type  Change 7 changes Regression 7 regressions
                 Current
#2867
     Baseline
#2835
Regression  JS 9.23MiB (+100%) undefined
Regression  CSS 868.11KiB (+100%) undefined
Regression  Fonts 526.1KiB (+100%) undefined
Regression  Media 295.6KiB (+100%) undefined
Regression  IMG 140.74KiB (+100%) undefined
Regression  HTML 1.38KiB (+100%) undefined
Regression  Other 871B (+100%) undefined

Bundle analysis reportBranch mherwege:model_drag_dropProject dashboard


Generated by RelativeCIDocumentationReport issue

@ghys
Copy link
Member

ghys commented Jan 3, 2025

Indeed and I can't imagine openHAB 5 without it :)

@ghys
Copy link
Member

ghys commented Jan 3, 2025

First comment from watching the GIFs alone would be to make sure there's a confirm message for every drag and drop as it might be too easy to screw up and make an involuntary change. This could be an additional dialog or a Cancel button to the existing ones - like the one asking whether to add an Equipment or a Point.

@mherwege
Copy link
Contributor Author

mherwege commented Jan 4, 2025

First comment from watching the GIFs alone would be to make sure there's a confirm message for every drag and drop as it might be too easy to screw up and make an involuntary change. This could be an additional dialog or a Cancel button to the existing ones - like the one asking whether to add an Equipment or a Point.

I have added some confirmation dialogs.

@mherwege
Copy link
Contributor Author

mherwege commented Jan 6, 2025

From my perspective, this is ready for review and testing by others.

@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels Jan 7, 2025
@florian-h05
Copy link
Contributor

@ghys Do you want to review?

@ghys
Copy link
Member

ghys commented Jan 8, 2025

@florian-h05 yeah if you don't mind, this is personal for me (like the log viewer) since I always wanted to do it but never got around to it. So I'm glad it got implemented by @mherwege and I'll take care of this one and get it done.
I think you have plenty on your plate already :)

@ghys
Copy link
Member

ghys commented Jan 12, 2025

@mherwege I'm testing it and unfortunately I do have a major remark which IMO is a must fix...

The tree view now adds arrows/chevrons besides items even if they don't have children, it's super confusing:

Before:
image

After:
image

This leads to unnecessary clicks on them because you'd think there's something to expand when there's not.

Actually it's the case for the sitemap tree view now too (I missed it).

I think it should be fixed because it leads to confusion for example:

image

Since the Text widget here is an "expanded empty group" we could easily think the Switch is one of its children, when it's actually on the same level as a child of the Frame.

image

It is possible to at least show those after you've started the drag operation only (so you can insert into an empty group)?

florian-h05 added a commit to florian-h05/openhab-webui that referenced this pull request Jan 12, 2025
@florian-h05
Copy link
Contributor

Actually it's the case for the sitemap tree view now too (I missed it).

Fixed in #3007.

@mherwege
Copy link
Contributor Author

@mherwege I'm testing it and unfortunately I do have a major remark which IMO is a must fix...

The tree view now adds arrows/chevrons besides items even if they don't have children, it's super confusing:

Yes, fair enough. And it looks like @florian-h05 already found a fix for it in the sitemap editor. I will look into doing something similar here, but need to find some time for doing it. It doesn't look to be a major change, so should be easy enough to include without changing everything. I don't think it should impact other review comments you may have.

@mherwege
Copy link
Contributor Author

I created a fix in this PR.

florian-h05 added a commit that referenced this pull request Jan 13, 2025
@florian-h05 florian-h05 removed their request for review January 13, 2025 12:29
@florian-h05
Copy link
Contributor

@ghys Can you please have another look at this?

Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Feb 28, 2025

@jimtng I think ESC now works.
I am still struggling with the performance aspect. The code is now full of debug logging. It logs timings from the first detection of a drag start. It looks like the time spent inside the code I developed is limited, and most time is spent before the first detection of a drag start and after finishing my part of the code, hence inside the Sortable.js code I suppose. I don't know what I can do about that. It does look like the performance is a lot worse if the structure is deeply nested.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Mar 3, 2025

@jimtng Can you check if the latest is more performant for you? I believe I improved it somewhat (without being perfect).

Signed-off-by: Mark Herwege <[email protected]>
@jimtng
Copy link
Contributor

jimtng commented Mar 3, 2025

@jimtng Can you check if the latest is more performant for you? I believe I improved it somewhat (without being perfect).

There seem to be a significant improvement in certain parts of the operation.
However, it seems that there are other parts of the operation that are still "strange" and slow. So what you said about it being more performant but not perfect is accurate.

@jimtng
Copy link
Contributor

jimtng commented Mar 3, 2025

I could somehow end up in this weird state, the only way to recover is refresh

image

The TestPoint1 that I was dragging, somehow ended up "frozen" on the screen like that even after I've released the mouse button. Unfortunately I don't know how to reliably cause this situation.

@mherwege
Copy link
Contributor Author

mherwege commented Mar 3, 2025

The TestPoint1 that I was dragging, somehow ended up "frozen" on the screen like that even after I've released the mouse button. Unfortunately I don't know how to reliably cause this situation.

Not much I can work with. If you have the development tools open, you should have quite extensive debug logging in the console. That would at least pinpoint where it is at when this happens.
I may ultimately remove some of this logging, but I think it is still useful now. It also has some logging of timings.

Signed-off-by: Mark Herwege <[email protected]>
@jimtng
Copy link
Contributor

jimtng commented Mar 3, 2025

This looks a bit strange

image

I'm dragging an equipment so this message shouldn't have showed up.

I think this "feature" shouldn't be offered at all. It has more potential for creating confusion and chaos. If someone needs this, they can set it up manually.

@jimtng
Copy link
Contributor

jimtng commented Mar 3, 2025

BTW The performance is great after the last commit. I haven't noticed any sluggishness / delays so far. Thanks for your work, well done!

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Mar 3, 2025

I'm dragging an equipment so this message shouldn't have showed up.

Indeed, I believe that should be fixed.

I think this "feature" shouldn't be offered at all. It has more potential for creating confusion and chaos. If someone needs this, they can set it up manually.

I am considering removing it. I just need to make sure I then don't loose anything if it was manually set and you start moving things around.

@jimtng
Copy link
Contributor

jimtng commented Mar 3, 2025

I just need to make sure I then don't loose anything if it was manually set and you start moving things around.

Maybe just do a warning if a point was originally in both equipment + location

But if a point only belongs to either equipment or location but not both, then move them as usual without retaining the original parent nor prompting about it.

@mherwege
Copy link
Contributor Author

mherwege commented Mar 3, 2025

Maybe just do a warning if a point was originally in both equipment + location

But if a point only belongs to either equipment or location but not both, then move them as usual without retaining the original parent nor prompting about it.

I implemented this suggestion.

@jimtng
Copy link
Contributor

jimtng commented Mar 4, 2025

I encountered another issue that didn't happen on main branch
Watch this animated gif, towards the end, the selection got "stuck" at Deebot and no matter what I tried, I couldn't click others to select them.

glitch-unselectable

It looks like at some point, a drag start was (mistakenly) triggered. I wasn't intentionally trying to drag at all in here. But that dragstart might've been the trigger that caused the inability to select any other items in the model.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Mar 4, 2025

It looks like at some point, a drag start was (mistakenly) triggered. I wasn't intentionally trying to drag at all in here. But that dragstart might've been the trigger that caused the inability to select any other items in the model.

Yes, I noticed this as well. I had put in some logic not to open the details when dragging, as this inadvertently happened without this, but missed the case when there is no move in the end. I fixed that.

@jimtng
Copy link
Contributor

jimtng commented Mar 4, 2025

With the latest update, I no longer experience "frozen" selection. But sometimes I still noticed "glitchy" item as I was clicking around (selecting different items). It looked like the item was about to be dragged, so it moved a bit, even though I wasn't trying to drag.

Maybe it's a bit too sensitive? Is there some sort of "mouse down and move a minimum of X pixels before it's considered a dragstart"? I'm not sure if this is the cause, so I'm just making a guess here.

@mherwege
Copy link
Contributor Author

mherwege commented Mar 4, 2025

Maybe it's a bit too sensitive? Is there some sort of "mouse down and move a minimum of X pixels before it's considered a dragstart"? I'm not sure if this is the cause, so I'm just making a guess here.

That's indeed the reason. I just changed it to have some move threshold.

@jimtng
Copy link
Contributor

jimtng commented Mar 4, 2025

Maybe it's a bit too sensitive? Is there some sort of "mouse down and move a minimum of X pixels before it's considered a dragstart"? I'm not sure if this is the cause, so I'm just making a guess here.

That's indeed the reason. I just changed it to have some move threshold.

This seems fine now.

I found another issue:

image

This should have never been allowed. It should just be ignored, and the mouse cursor changed to a circle with diagonal line not-allowed

image

I think it doesn't currently allow it to be dropped into a read-only (file-based) semantic groups, so perhaps just use that same mechanism (instead of changing the cursor)

@mherwege
Copy link
Contributor Author

mherwege commented Mar 4, 2025

I think it doesn't currently allow it to be dropped into a read-only (file-based) semantic groups, so perhaps just use that same mechanism (instead of changing the cursor)

An editable item should be allowed to be dropped in a non-editable semantic group, as the changes will only need to be persisted for the item, not the group you drop into. But I need to think this through in more detail, as I have not spent much time on this corner case.

This should have never been allowed. It should just be ignored, and the mouse cursor changed to a circle with diagonal line not-allowed

I tried to make this more dynamic at one point, but failed, so settled for a warning. I can try again. A warning is still better than silently reverting in my view.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Mar 5, 2025

I tried to make this more dynamic at one point, but failed, so settled for a warning. I can try again. A warning is still better than silently reverting in my view.

It doesn't allow dropping now where not allowed. It would allow dropping a managed item into a non-managed group, which I believe should be possible. I also added a 1s delay in group opening, so you can move over a group without it being opened if you do not want to drop inside of it.

While this may be an improvement, I am still not 100% convinced about this. First, I didn't succeed in changing the cursor. Second, if you move over a place where you can drop and then a place where you cannot, it will drop in the last place where it could drop. And that may be well away from the current cursor position.

EDIT: the second point is solved.

mherwege added 2 commits March 5, 2025 09:29
Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Mar 6, 2025

@jimtng I have spent a lot of time trying to change the cursor when moving over an item where drop is not allowed. At the moment, it will not drop and revert (without a message), but the cursor does not change. I have tried many things, but couldn't find a solution. Looking at the issues and discussions online, it looks like many people are not able to successfully do this. I give up on this aspect.

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
5 participants