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

Moving Collection index inspector section to declarative UI #1107

Conversation

KRLango
Copy link
Contributor

@KRLango KRLango commented Jul 24, 2024

As part of 1093, moving the collection index inspector section to declarative UI

@KRLango
Copy link
Contributor Author

KRLango commented Jul 24, 2024

Waiting on nion-software/nionutils#26

@cmeyer
Copy link
Collaborator

cmeyer commented Jul 24, 2024

I don't think this use of converters is good - the side effects of accessing a second object are difficult to reconcile.

For this PR, can you move the proposed nionutils classes directly into Inspector.py? Then we have a single place to replace them with a better solution and don't risk anyone else using that technique.

I'll propose an alternate solution, but it will take a few days.

@cmeyer
Copy link
Collaborator

cmeyer commented Jul 24, 2024

Ok - not a few days - we can use a model instead of a converter. It's a very clean solution that doesn't use tuple converters. I put the commit in my repo, look at the latest commit on the link below.

https://github.com/cmeyer/nionswift/commits/review-1107/

@KRLango
Copy link
Contributor Author

KRLango commented Jul 26, 2024

@cmeyer my concern with using the model rather than a converter is reusability. I agree that the converter is not the cleanest, but the code you put in the model would have to be rewritten almost verbatim every time we come across a similar situation.

@cmeyer
Copy link
Collaborator

cmeyer commented Jul 26, 2024

I haven't had a reason to use a tuple converter yet - so it's not a frequent situation. There might be a few other cases, but it won't be many.

Can you rebase this PR on the latest nionswift and force push to address the mypy issues?

@cmeyer
Copy link
Collaborator

cmeyer commented Jul 26, 2024

A note: you can do a merge with the trunk instead of a rebase to keep the commits intact during review and not require a "force push". Do this with a get merge master on the branch, then git push normally to this PR.

@cmeyer
Copy link
Collaborator

cmeyer commented Jul 26, 2024

Follow-up comment: if you're concerned about reusability, we can add a "tuple value model" or something similar. However, before putting it in nionutils, we can try it in nionswift first. And before trying it in nionswift, we can wait for a 2nd occurrence of it to be needed.

@KRLango
Copy link
Contributor Author

KRLango commented Jul 29, 2024

You are far more familiar with the codebase than I am so I am happy to concede to your experience that there are no other places you have needed to use this and put my reusability concerns aside unless we do find other places we want to do this again.

@KRLango KRLango marked this pull request as ready for review July 29, 2024 10:02
@cmeyer
Copy link
Collaborator

cmeyer commented Jul 31, 2024

Merged afa45e3 Thanks!

@cmeyer cmeyer closed this Jul 31, 2024
@KRLango KRLango deleted the 1093-CollectionIndexInspectorSection branch August 1, 2024 14:51
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.

None yet

2 participants