Fix issues with mouse cursor position when editing nodes #7014
Conversation
vitvakatu
requested review from
MichaelMauderer,
wdanilo,
farmaazon and
kazcw
as code owners
| *self.label_layer.borrow_mut() = layer.downgrade(); | ||
| // We're taking extra measures to ensure that the actual layer of the label will be updated | ||
| // even if it is already displayed. It should be unnecessary with the current | ||
| // implementation. See [`Self::show_edit_mode_label`] and [`Self::hide_edit_mode_label`]. |
There was a problem hiding this comment.
It should be unnecessary with the current implementation.
I don't understand this. If it's unnecessary, why do we do this?
There was a problem hiding this comment.
Well, we have no control over when the set_label_layer method will be called from outside of this component, and technically it can happen when we already have the label visible. As far as I understand the code, it shouldn't happen, but one can refactor it in the future and accidentally change the behavior. So it is just a preventative measure.
There was a problem hiding this comment.
I think the docs rephrased this way would be more understandable to me:
Currently, we never sets label layer when it's already displayed, but - as `set_label_layer` is a public method of this component - we're taking extra measures.
There was a problem hiding this comment.
- Adam asked a question because the comment in the code is confusing. I see that the code did not change (the docs are not improved / expanded) and it is still confusing, so the question was not addressed yet. The comment is also very confusing to me – if it "should be unnecessary with the current implementation", it should not be there.
- You have full control of if the
set_label_layerwill be called from outside of this component – the method is private, so it will not be called from outside.
I don't think we should include "dead code" that does nothing and is there just in case someone refactors code without thinking about the logic. This makes the whole code hard to understand and follow. In the future, when browsing this code, I would probably spend a lot of time trying to understand why edit_mode_label_displayed is needed and why we are managing this state, thinking that probably this solves some issue. If it is not needed, lets not add it :)
There was a problem hiding this comment.
You have full control of if the set_label_layer will be called from outside of this component – the method is private, so it will not be called from outside.
This is not true, it is called from the public method of view.
There was a problem hiding this comment.
Github have fooled me. I actually added a comment yesterday, but it was kept in a pending state, and I didn't notice.
Now it should be visible. I proposed an alternative comment. Check @wdanilo if it's ok to you.
There was a problem hiding this comment.
Thank you, I like your proposal.
QA: 🟢 |
CHANGELOG.md
Outdated
| @@ -183,6 +183,7 @@ | |||
| execution failed][6918]. | |||
| - [Performance and readability of documentation panel was improved][6893]. The | |||
| documentation is now split into separate pages, which are much smaller. | |||
| - [Fixed cursor position when ctrl-clicking the node][7014]. | |||
There was a problem hiding this comment.
These are user-facing logs. Users reading them should understand what changed without the need to click something and without prior knowledge. Telling "fixed cursor position" does not really explain what changed to the reader. I'd be thankful If you could expand that a little bit.
| *self.label_layer.borrow_mut() = layer.downgrade(); | ||
| // We're taking extra measures to ensure that the actual layer of the label will be updated | ||
| // even if it is already displayed. It should be unnecessary with the current | ||
| // implementation. See [`Self::show_edit_mode_label`] and [`Self::hide_edit_mode_label`]. |
There was a problem hiding this comment.
- Adam asked a question because the comment in the code is confusing. I see that the code did not change (the docs are not improved / expanded) and it is still confusing, so the question was not addressed yet. The comment is also very confusing to me – if it "should be unnecessary with the current implementation", it should not be there.
- You have full control of if the
set_label_layerwill be called from outside of this component – the method is private, so it will not be called from outside.
I don't think we should include "dead code" that does nothing and is there just in case someone refactors code without thinking about the logic. This makes the whole code hard to understand and follow. In the future, when browsing this code, I would probably spend a lot of time trying to understand why edit_mode_label_displayed is needed and why we are managing this state, thinking that probably this solves some issue. If it is not needed, lets not add it :)
|
@wdanilo can you re-review this? |
|
@vitvakatu What's the status here? It's 4 days old w/o any action. |
|
@xvcgreg blame engine CI on windows, I don't know what has happened to it. Restarting one more time... |
|
@vitvakatu in case of random failures, ping me and I will merge it :) |
Pull Request Description
Fixes #6702
The issue was in the code for hiding/showing the node label. When showing it for the first time, the position of the label was updated only in the next frame, so the whole calculation was incorrect.
It is fixed using the
DETACHEDscene layer to hide the label when needed. Its position is always correct.2023-06-12.14-42-45.mp4
Important Notes
I added a bit of additional code to ensure we always use the correct layer. The label is a complicated entity, as it lives in different layers in different application states. Also, I removed the code for setting the initial layer from the
initmethod, as it seems unused.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.