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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Library editor: Use GraphicsScene::itemsBoundingRect() to select all items #1243

Merged
merged 1 commit into from Oct 2, 2023

Conversation

BuFran
Copy link
Contributor

@BuFran BuFran commented Oct 2, 2023

The currentGraphicsItem is always empty (there is no insert anywhere) so it returned always empty rect.

After this PR, for selectAll functionality, the entire graphicsScene for retrieving entire content rect is used.

Maybe the solution is wrong, probably I missed intent of currentGraphicsItem. The board and schematic does this differently.

One architecture misconception is that on symbol it is reference, on package it is shared pointer so probably there is really unfinished rework.

There should be no style bugs because I tried to run dev/format_code.sh 馃槂


Added by @ubruhin:

Fixes #1179

@ubruhin
Copy link
Member

ubruhin commented Oct 2, 2023

I think the code makes sense 馃憤 Will quickly test it soon.

One architecture misconception is that on symbol it is reference, on package it is shared pointer so probably there is really unfinished rework.

No, this is on purpose. A symbol always consists of exactly one graphics item (the symbol) so we can use a reference. But a package consists of 0..n footprints, and thus 0..n graphics items. In case of 0 footprints, we don't have any graphics to display so it will be nullptr.

There should be no style bugs because I tried to run dev/format_code.sh 馃槂

Awesome :) By the way, since you are contributing regularly, I may tell you some more things about our preferred workflow 馃檪

We prefer commit messages according this recommendation (see also CONTRIBUTING.md). It may sound picky, but it really helps to keep the Git log readable and messages easy to understand if every contributor follows the same style.

For example the GitHub issue should not be mentioned in the first line of the commit message. Just describe what you did in this commit (e.g. "Library editor: Use GraphicsScene::itemsBoundingRect() to select all items"). If the commit closes a particular GitHub issue, just write "Closes #nnn" at the end of the PR body - this will make GitHub to close the issue automatically when merging the PR so we don't have to do it manually.

So far our preference, but especially for simpler PRs where a single commit is enough, don't worry too much about it. This is also an advantage of squash-merge that the final commit message can easily be adjusted in the GitHub UI 馃檲

@ubruhin ubruhin changed the title Select-all functionality in library (fixing #1179) Library editor: Use GraphicsScene::itemsBoundingRect() to select all items Oct 2, 2023
@ubruhin ubruhin added bug UI / UX User Interface/Experience tool-library-editor labels Oct 2, 2023
@ubruhin ubruhin added this to the 1.1.0 milestone Oct 2, 2023
@ubruhin
Copy link
Member

ubruhin commented Oct 2, 2023

Seems to work fine, thanks!

@ubruhin ubruhin merged commit b4a8640 into LibrePCB:master Oct 2, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug tool-library-editor UI / UX User Interface/Experience
Development

Successfully merging this pull request may close these issues.

Select-all in library editor only selects items within 14 squares of the center
2 participants