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

Fix #651. Add title manager to ensure unique displayed titles. #1221

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmeyer
Copy link
Collaborator

@cmeyer cmeyer commented Oct 22, 2024

How I tested:

Use Processing > Generate to create 2+ data items with same name. Take FFT of results to generate new data items. Run acquisition and take multiple snapshots and see that they have unique titles.

@cmeyer cmeyer force-pushed the fix-651-unique-title-manager branch from debf282 to d0cf416 Compare October 22, 2024 19:35
@cmeyer cmeyer marked this pull request as draft October 22, 2024 20:57
@cmeyer cmeyer force-pushed the fix-651-unique-title-manager branch from d0cf416 to f701421 Compare October 23, 2024 02:29
@cmeyer cmeyer marked this pull request as ready for review October 23, 2024 03:24
Copy link
Contributor

@KRLango KRLango left a comment

Choose a reason for hiding this comment

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

I have concerns about this PR.

What is the problem it is trying to solve?
Having unique names - but why do users want unique names, and does this solve their actual need?

During my testing, I got 2 identical suffixes #96 with 4 snapshots. While I accept this will be rare - how often do we want to accept collisions happening?

Will the random nature of the suffixes confuse users? For example, the first few I got all appeared to be just numbers #58, #96, #84. Are users going to wonder what is going on with those numbers?

I believe the implementation done for the Acquisitions is a much cleaner user experience, and I think something similar would work better here.

@cmeyer
Copy link
Collaborator Author

cmeyer commented Oct 23, 2024

This may not be needed in light of the acquisition index change and it may indeed be confusing to users. We can discuss that before merging (or not merging).

However, I'm concerned that even if we decided it was a useful feature, it shouldn't ever have duplicate suffixes. So I'm wondering how that occurred. I tested by changing the minimum length to 1 and then creating multiple snapshots and when it got to a duplicated suffix out of the 0-9A-F set, it increased the length to 2. Did this happen when the '96' appeared for you, e.g. increase to length 3 to distinguish between the two '96's?

@KRLango
Copy link
Contributor

KRLango commented Oct 23, 2024

I have gone back to the project I used for this. I did some snapshots using ctrl + s and some using the menu. When I found the 2 96's I think it was one with each since the names are slightly different (Display Snapshot vs Snapshot).

{4BBBCB56-BE3B-48B9-8B3E-935582C87E9D}

@cmeyer
Copy link
Collaborator Author

cmeyer commented Oct 23, 2024

Ok that makes sense - but it does highlight a problem with this scheme. I'm going to change this to draft while I think it through.

@cmeyer cmeyer marked this pull request as draft October 23, 2024 16:59
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