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

[Gui] Issue #3923 - Add arbitrary Tree View item ordering #4626

Merged
merged 1 commit into from Nov 11, 2021

Conversation

pavltom
Copy link
Contributor

@pavltom pavltom commented Mar 16, 2021

This pull request gives the user a possibility to reorder the document tree view items.

For more implementation details please see my contribution to the dedicated forum topic:

Ticket #3923 - change order in tree view

@luzpaz luzpaz added Translation Translation and localization related and removed Translation Translation and localization related labels Mar 16, 2021
@adrianinsaval
Copy link
Member

Compiles and runs but there are issues with the implementation I have commented here: https://forum.freecadweb.org/viewtopic.php?f=3&t=35316&p=488236#p488236

@ickby
Copy link
Contributor

ickby commented Mar 17, 2021

The viewprovider tree rank is not readable, writable or observable from python. As it is a defining attribute for the viewprovider this needs to change.
Maybe implementing it as hidden property makes it easier and more consistent within freecad?

@thebravoman
Copy link

For what it is worth, I've been in the CAD/Engineering world for decade, I installed FreeCAD and the first thing I did is add two parts and tried to reorder the parts in the tree viewer.

Looking forward to this PR

@ickby
Copy link
Contributor

ickby commented Apr 24, 2021

Thanks for the changes, looks fine for me! Two minor things that could be improved:

  1. Use the new functionality for App::Group and App::Part as those are most likely the two candidate objects for the new API
  2. Write a test for the functionality to check if reordering works and is prevented correctly if it should not work

@pavltom
Copy link
Contributor Author

pavltom commented Jun 3, 2021

Hi, I have used the possibility to disable children movement for basic document objects like Body (and all OriginGroupExtensions), Origin document object, Part Boolean object, Loft, Sweep (Pipe) and Thickness features both in Part and PartDesign. For the App::DocumentObjectGroup object, movement remains allowed, I do not see any reason why it should be restricted. Nevertheless, I am pretty much sure there are lots of other places, where moving children is not desirable, however I would let apply this to their respective maintainers.
As for the tests, I have added a TreeView.py test, which tests the functionality both for items allowing and restricting child item movement. I believe this test is sufficient, but for sure it can be expanded to other item types or different scenarios.

@berndhahnebach berndhahnebach added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Aug 3, 2021
@berndhahnebach
Copy link
Contributor

berndhahnebach commented Sep 24, 2021

Following a link to the branch on the CI-repository:

https://gitlab.com/freecad/FreeCAD-CI/-/commits/PR_4626

The CI-status is available on the latest commit of the branch.
If there is no status available the PR should be rebased on latest master.
Check pipeline branches to see if your PR has been run by the CI.

https://gitlab.com/freecad/FreeCAD-CI/-/pipelines?scope=branches

Copy link
Member

@chennes chennes left a comment

Choose a reason for hiding this comment

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

Nice addition! Thanks especially for adding the test cases. I left a couple tiny comments on the code (which overall looks really solid), and some feedback on the overall behavior.

When I move an expanded group up the list, it stays open, but when I move it down, it collapses. This is also true if I move another object "across" the open group: it collapses if the motion is upward, but stays open if the motion is down. I personally prefer the stay-open behavior, but at the least I think it should be the same in all cases.

I also think that the multiple-selection behavior is counter-intuitive. When I have multiple objects selected and ask them to move, I felt like I was asking ALL of them to move, and then only one of them did.

Otherwise it looks to me like this is pretty much ready to merge. Thanks!

: Command("Std_GroupMoveUp")
{
sGroup = QT_TR_NOOP("TreeView");
sMenuText = QT_TR_NOOP("Move Up in Group");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sMenuText = QT_TR_NOOP("Move Up in Group");
sMenuText = QT_TR_NOOP("Move up in group");

: Command("Std_GroupMoveDown")
{
sGroup = QT_TR_NOOP("TreeView");
sMenuText = QT_TR_NOOP("Move Down in Group");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sMenuText = QT_TR_NOOP("Move Down in Group");
sMenuText = QT_TR_NOOP("Move down in group");

@freecadci
Copy link

pipeline statusfor feature branch PR_4626. Pipeline #394851880 was triggered at 214972f. All CI branch pipelines.

@AjinkyaDahale
Copy link
Contributor

For what it is worth, I've been in the CAD/Engineering world for decade, I installed FreeCAD and the first thing I did is add two parts and tried to reorder the parts in the tree viewer.

Looking forward to this PR

I wouldn't me surprised if 90% of first contributions come from using other CAD software and finding a missing feature. I know most of my contribs are such.

@pavltom
Copy link
Contributor Author

pavltom commented Oct 26, 2021

I have reworked the functionality a bit, so now moving multiple siblings in a single step is possible. Also Undo/Redo shall be now fully supported. Finally, I have added one more Python test to cover the new item order survives closing the document once it was saved. However keeping the "historical" commits would be quite misleading, thus I decided to rebase the branch. Should there be any issues, please let me know.

@freecadci
Copy link

pipeline status for feature branch PR_4626. Pipeline 396028972 was triggered at 345dcd5. All CI branches and pipelines.

@FreeCAD FreeCAD deleted a comment from freecadci Oct 26, 2021
@FreeCAD FreeCAD deleted a comment from freecadci Oct 26, 2021
@HakanSeven12
Copy link
Contributor

That is nice 👍 can you expose this functions to Python too?

@pavltom
Copy link
Contributor Author

pavltom commented Oct 28, 2021

@HakanSeven12 You can use Python to select the objects to be reordered and then run command "Std_GroupMoveUp"/"Std_GroupMoveDown". For working examples please see the Python tests in "src/Mod/Test/TreeView.py" which are a part of this pull request.

@chennes chennes self-assigned this Oct 31, 2021
@chennes
Copy link
Member

chennes commented Oct 31, 2021

Thanks for the changes -- I will re-review shortly.

@chennes
Copy link
Member

chennes commented Nov 19, 2021

@pavltom It appears this is causing some problems, can you please have a look at https://forum.freecadweb.org/viewtopic.php?f=3&t=63744 ? Thanks.

chennes added a commit that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants