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

Refactor Component::getTextSelected #708

Merged
merged 2 commits into from
May 25, 2024

Conversation

wawuwo
Copy link
Contributor

@wawuwo wawuwo commented Apr 26, 2024

Hi!

This one refactors the function responsible for determining which text property of a component is clicked:

  • obscure Corr arg indirectly coming from Schematic::textCorr is removed
  • function body is rewritten in a more clear and easy to understand way
  • function is documented
  • semantics remain the same.

If something is affected by these changes that would be the "click-on-component-property" behaviour; I checked it before pushing and it looked like it was before.

Semantics remain the same. Function body is documented and rewritten
in more clear way.
@ra3xdh ra3xdh added this to the 24.3.0 milestone May 3, 2024
@ra3xdh
Copy link
Owner

ra3xdh commented May 23, 2024

I have tested this PR. Something works wrong with components having long properties list like library transformer device. Look at the attached screencast. The invisible properties are selected. I am using Qt6.7.0+Wayland.
Screencast_20240523_091320.webm

@wawuwo
Copy link
Contributor Author

wawuwo commented May 25, 2024

@ra3xdh I've fixed it, that was my mistake: while iterating over component's properties the index number describing current property wasn't increased if the property was hidden. It made the index number "lack behind" and point to a wrong property. The bug was not related to a long properties lists, it could be reproduced with any component: one just had to make a "gap" in properties (i.e. "visible, visible, invisible, visible, visible") and click on any property after the gap.

I hope it'll be OK now! 😎

@ra3xdh ra3xdh merged commit 3a090d8 into ra3xdh:current May 25, 2024
7 checks passed
@ra3xdh
Copy link
Owner

ra3xdh commented May 25, 2024

Thanks for the contribution! Now everything works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants