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

1093- New Models useful for graphics section #1132

Closed

Conversation

KRLango
Copy link
Contributor

@KRLango KRLango commented Aug 14, 2024

This is to add 2 new models that I am finding will be useful for the graphics section of the inspector work:

TuplePropertyElementModel - many of the graphics sizes and positions are stored in tuples, this model is to access a single element of that tuple and to update the tuple when the value is changed.

DisplayItemCalibratedValueModel - the sizes and positions of the graphics can be displayed in a number of different units and this model watched for the calibration to be changed to update the values in the UI

Copy link
Collaborator

@cmeyer cmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem you mentioned in chat was caused by the binding not keeping a reference to the source. This was a historical choice and I've changed it in nion-software/nionutils@f9ff65b.

I still show a couple tests failing.

Also, one minor change request I noted in the code: can you use double underscore for most private fields and callbacks? I recommend only using single underscore for protected fields, and even in that case I usually try not to expose the variable directly and instead provide a read-only property.

Example of method that will never be called in subclass and should be private (double underscore):
DisplayItemCalibrationValueModel._on_calibration_changed -> DisplayItemCalibrationValueModel.__on_calibration_changed

Example of field that should not be used in subclasses and should be private:
TuplePropertyElementModel._listener -> TuplePropertyElementModel.__listener.

By default I would make almost all fields private unless there is a compelling reason that a subclass needs access.

Once you make the private fields, let me know if you want me to merge or want to keep adding to this PR.

@cmeyer
Copy link
Collaborator

cmeyer commented Aug 15, 2024

NOTE: you will want to merge from the main branch before pushing another PR, to get the numpy 2 changes, which are required to utilize the new nionutil changes. Or rebase and force push.

@KRLango KRLango marked this pull request as ready for review August 15, 2024 13:11
@KRLango
Copy link
Contributor Author

KRLango commented Aug 15, 2024

Should be all good now. If possible I would like to get this one merged quickly as it will be useful with the Graphics Inspector section that I am currently working on.

@cmeyer
Copy link
Collaborator

cmeyer commented Aug 15, 2024

Squashed and merged here f3e67d8

Thanks! This looks like a good approach going forward (using models instead of custom bindings). We may want to move TuplePropertyElementModel to nionutils.Model eventually, but let's keep it in nionswift until the inspector project is complete. I'd also rename it to TuplePropertyModel if we did that - is there a reason for the Element in there? (no need to answer here, just something to consider)

@cmeyer cmeyer closed this Aug 15, 2024
@KRLango
Copy link
Contributor Author

KRLango commented Aug 16, 2024

@cmeyer the reason I put element in there was because the model was to access an element of the tuple property rather than the whole tuple.

@cmeyer
Copy link
Collaborator

cmeyer commented Aug 16, 2024

Wouldn't it be called TupleElementPropertyModel then? I don't think there would be any ambiguity with just TuplePropertyModel since there isn't any difference between a "whole tuple property model" and just a "property model", but I guess I can kind of see the logic. But TuplePropertyElementModel just seems incorrect to me.

@KRLango KRLango deleted the 1093-CalibratedValueModel branch August 21, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants