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 library element context menu #380

Merged
merged 2 commits into from Dec 2, 2018

Conversation

Projects
None yet
2 participants
@dbrgn
Copy link
Member

commented Nov 18, 2018

Add context menu entry for removing the currently selected element.

Refs #200.

@ubruhin this is a first draft. Is the way I approached the issue correct? If yes, I can add support for the other library elements.

This could also be extended later on, e.g. when right-clicking on the empty space of the symbol list widget, we could add an entry called "Create symbol". When right-clicking on an existing symbol, we could add an entry called "Create symbol from this symbol" or something like that.

I think once this is in place, we can actually remove the delete menu entries, I think they're bad UX because it's not clear what they're referring to.

@dbrgn dbrgn requested a review from ubruhin Nov 18, 2018

@ubruhin

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

@ubruhin this is a first draft. Is the way I approached the issue correct? If yes, I can add support for the other library elements.

Looks not bad so far 👍 Just don't forget that a library element may be opened at the time of its deletion, in that case the corresponding tab needs to be closed first.

This could also be extended later on, e.g. when right-clicking on the empty space of the symbol list widget, we could add an entry called "Create symbol". When right-clicking on an existing symbol, we could add an entry called "Create symbol from this symbol" or something like that.

👍

I think once this is in place, we can actually remove the delete menu entries, I think they're bad UX because it's not clear what they're referring to.

Would be nice if you could do it in this PR :)

@dbrgn dbrgn force-pushed the library-editor-contextmenu branch from 9a8d18f to 8ad587f Nov 18, 2018

@dbrgn

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

@ubruhin do you know how to prevent the following bug?

https://tmp.dbrgn.ch/librepcb-2018-11-18_23.06.34.webm

For some reason, the selection is not updated sometimes when right-clicking on another item while the context menu for the first item is still open.

@dbrgn

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

Regarding closing the tab if it's open:

  • This is something I'd have to do in the libraryeditor.cpp, right? So I'd
    register a slot there, and emit an event after deleting the item in
    libraryoverviewwidget.cpp?
  • How do I find out whether the element was opened in a tab?

I removed the remove action in the file menu, it's now replaced with the context menu. However, regarding the disabling of the delete/rotate items in the edit menu and toolbar, I think that's complex enough that it should be done in a separate PR. It doesn't have anything to do with removing library elements.

@dbrgn

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

PR is now refactored and uses itemAt to get the currently selected item. Furthermore, instead of the sender, he selected item text as well as the file path are passed to the removeSelectedItem method.

@dbrgn dbrgn force-pushed the library-editor-contextmenu branch from b043e5e to 047cba3 Nov 29, 2018

@ubruhin
Copy link
Member

left a comment

See comments. In addition, the branch should be rebased.

Btw, it would be very nice to also support the "delete" keyboard shortcut to remove the currently selected item, although it's not required to get this merged :)

@dbrgn dbrgn force-pushed the library-editor-contextmenu branch from 047cba3 to e83a022 Nov 30, 2018

@dbrgn

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

Ok, only issue left is now the closing of the open tab.

@dbrgn dbrgn force-pushed the library-editor-contextmenu branch from 43e1990 to 313b59b Dec 1, 2018

@dbrgn

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

Ok, all requested issues fixed. If you want you can already review, but I also want to add a functional test 🙂

@ubruhin
Copy link
Member

left a comment

Looks very good now! Only two small things which could be improved, but I would also merge it the way it is now :)

Show resolved Hide resolved libs/librepcb/libraryeditor/lib/libraryoverviewwidget.cpp Outdated
Show resolved Hide resolved libs/librepcb/libraryeditor/libraryeditor.h Outdated
@ubruhin

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Ah, I forgot: If you have clang-format 6.x installed, it would be nice if you could run ./dev/format_code.sh to format the code ;)

@ubruhin ubruhin added this to the 0.1.1 milestone Dec 1, 2018

dbrgn added some commits Nov 18, 2018

Add library editor context menus
Add context menu entry for removing the currently selected element in
the library element overview.
LibraryEditor: Remove "File > Remove" action
The context menu entry is less ambiguous.

@dbrgn dbrgn force-pushed the library-editor-contextmenu branch from 2504903 to 43fdcf4 Dec 2, 2018

Did requested changes

@dbrgn

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2018

Functional tests are waiting for parkouss/funq#47, so I think this can be merged.

@ubruhin ubruhin changed the title WIP: Add library element context menu Add library element context menu Dec 2, 2018

@ubruhin

This comment has been minimized.

Copy link
Member

commented Dec 2, 2018

Thanks!

@ubruhin ubruhin merged commit f4f981f into master Dec 2, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ubruhin ubruhin deleted the library-editor-contextmenu branch Dec 2, 2018

ubruhin added a commit that referenced this pull request Mar 17, 2019

Merge pull request #380 from LibrePCB/library-editor-contextmenu
Add library element context menu
(cherry picked from commit f4f981f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.