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

LineageRegistrationPlugin: add copy spot labels functionality #61

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

maarzt
Copy link
Contributor

@maarzt maarzt commented Jun 14, 2024

issue #60

@maarzt maarzt self-assigned this Jun 19, 2024
@maarzt maarzt requested a review from stefanhahmann June 19, 2024 08:42
Copy link
Collaborator

@stefanhahmann stefanhahmann left a comment

Choose a reason for hiding this comment

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

I tested the function and it mainly worked as expected with one inconsistency:

Currently, the branch graph shows the label of the last spot that belongs to it, making the label of the last spot the significant label of the branch. This copy label function uses the label of the first spot to copy it to all spots of equivalent branch in the other project. This is okay, as long as users only work on the branch graph, but may lead to inconsistent / unexpected behavior, if the users use both, the branch graph and the spot graph.

I recommend some action to overcome this inconsistency.

{
for ( final Spot spotA : registration.mapAB.keySet() )
{
boolean hasLabel = !Integer.toString( spotA.getInternalPoolIndex() ).equals( spotA.getLabel() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, it would be a good idea, if there was a method in the Spot class, which can be used to test, if a label different to the internal pool index exists.

This could maybe even be part of the HasLabel interface.

public boolean isLabelSet()
	{
		return pool.label.isSet( this ); // preferable?
		// return !Integer.toString( getInternalPoolIndex() ).equals( getLabel() );
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened a PR mastodon-sc/mastodon#318 that addresses this suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@maarzt mastodon-sc/mastodon#318 has been merged in the meantime. Thus you could update here.

@@ -176,9 +184,12 @@ public LineageRegistrationFrame( Listener listener )
add( new JLabel( "Sort TrackScheme:" ) );
add( newOperationButton( "project A", SORT_TRACKSCHEME_TOOLTIP, listener::onSortTrackSchemeAClicked ), "split 2" );
add( newOperationButton( "project B", SORT_TRACKSCHEME_TOOLTIP, listener::onSortTrackSchemeBClicked ), "wrap" );
add( new JLabel( "Copy spot labels:" ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering, if Copy spot labels is the best term here.

The tool tip says that the function works on cell / branch level. I thus ask myself, if copy cell or branch label would be a better choice here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The text says now "copy spot labels"

@maarzt maarzt merged commit afb5805 into master Jul 2, 2024
1 check passed
@maarzt maarzt deleted the lineage-registration-copy-spot-labels branch July 2, 2024 15:36
@maarzt maarzt restored the lineage-registration-copy-spot-labels branch July 2, 2024 15:36
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