-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: offer possibility to editing view provider to handle "Select All" command #13929
Conversation
@@ -2588,6 +2588,12 @@ void ViewProviderSketch::updateColor() | |||
editCoinManager->updateColor(); | |||
} | |||
|
|||
bool ViewProviderSketch::selectAll() | |||
{ | |||
// TODO: eventually implement "select all" logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why leaving this as a TODO? It seems rather straight forward. Unless you want to split in 2 PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot of reasons:
- I have limited spare time
- I prefer quality over quantity. So I prefer to deliver a well tested and finalized PR rather than a bigger crappy one that I eventually won't be able to fix myself (see previous point)
- I agree with that part of the process that PR should always aim at fixing a minimal problem. Here it is. Especially the goal here is to solve a "bug" (weird behavior), not to implement a feature. So better have 2 PR.
- Even if it may sound obvious, probably better to discuss what "Select All" in Sketcher is expected to do
Related issue: #12746 So this PR won't make it possible to properly use Ctrl+A in sketches yet? I wanted to test this but git clone asks me for GitHub credentials. Is this branch protected somehow? |
Correct. But will be one step to it. 😉
Shouldn't. What git command are you using? |
Thanks for clarifying.
Now I know what happened, I just misspelled your username. I used O instead of 0. Sorry for the confusion ;-) Strange that it didn't just throw an error that the branch and repo don't match. |
OK. On my side I never use the origin branch but always the PR one with something like:
If you just checkout FC repo and don't have a fork, use |
@0penBrain Currently, I always use:
as advised here but I know it's ineffective. I need to become more familiar with git. |
Problem solved here is that, in the way the "Select all" command is handled, it always acts at document level selecting all objects in the tree.
It leads to strange behavior when editing a sketch and pressing "Ctrl+A": nothing in the sketch is selected, but all document objects are (behind the scenes) which triggers unexpected tool activation and so on.
This PR solves this is a flexible manner by implementing a mechanism that will, if a view provider is currently in edit mode, let it decide if it wants to handle the command or not.
In the actual PR, the VPSketch just gets the command and tells the viewer it is handled there, so that strange behavior is gone.
It is let for future work to implement the "Select All" logic in the VPSketch.