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

Add "Duplicate" button to duplicate display with same config #977

Merged
merged 1 commit into from
Mar 22, 2016

Conversation

wkentaro
Copy link
Contributor

@wkentaro wkentaro changed the title Add copy button to duplicate displays Add copy button to duplicate display with same config Mar 14, 2016
@wkentaro
Copy link
Contributor Author

ping

@NikolausDemmel
Copy link
Contributor

This seems like a very useful addition. Havent tested it, but high level review lgtm.
+1

@wjwwood
Copy link
Member

wjwwood commented Mar 22, 2016

@wkentaro thanks for the pr, this is a great feature!

I didn't, however, get a new display name (copy of ...) and so I opened a pull request for that: wkentaro#1

Also I'd like to ask you to consider changing the use of the term "copy" with "duplicate". Since you're not exactly copying it in the sense of copy and then paste zero to many times. You're really duplicating an item exactly one time.

So if you choose to do "duplicate" instead of "copy", then I'd recommend changing the new display name to be "duplicate of X", change the button to "duplicate" rather than "copy" and consider changing the hotkey of ctrl-c to ctrl-d (or something like that).

Thanks again.

@wkentaro
Copy link
Contributor Author

I didn't, however, get a new display name (copy of ...) and so I opened a pull request for that: wkentaro#1

I rethought about the name of copied display, and changed it to be same as the original one.
Which one do you prefer?

@wkentaro
Copy link
Contributor Author

Also I'd like to ask you to consider changing the use of the term "copy" with "duplicate". Since you're not exactly copying it in the sense of copy and then paste zero to many times. You're really duplicating an item exactly one time.
So if you choose to do "duplicate" instead of "copy", then I'd recommend changing the new display name to be "duplicate of X", change the button to "duplicate" rather than "copy" and consider changing the hotkey of ctrl-c to ctrl-d (or something like that).

It's reasonable, I use "duplicate" term.

@wkentaro wkentaro closed this Mar 22, 2016
@wkentaro wkentaro deleted the copy-display branch March 22, 2016 07:01
@wkentaro wkentaro restored the copy-display branch March 22, 2016 07:02
@wkentaro wkentaro reopened this Mar 22, 2016
@wkentaro wkentaro changed the title Add copy button to duplicate display with same config Add "Duplicate" button to duplicate display with same config Mar 22, 2016
@wkentaro
Copy link
Contributor Author

@wjwwood I have updated the commit.

@wjwwood
Copy link
Member

wjwwood commented Mar 22, 2016

@wkentaro looks good thanks. I'm testing it out now.

As for the new name, I think having something different is a good idea, but I think a suffix or prefix that is too long might not be best, e.g. "duplicate of X" or "X duplicate" or even "X dupe". I thought about suffixing numbers, e.g. "X (1)", then "X (2)", but the logic to get this right is tricky (and to avoid overmatching something the user created). Something simple could be to use the mathematical prime symbol to indicate a duplicate: https://en.wikipedia.org/wiki/Prime_(symbol) That would be easy to implement as you just add a single quote to the end of the name.

Anyone else have thoughts?

@NikolausDemmel
Copy link
Contributor

I woudn't mind having the duplicates with the exact same name. That is what rviz does already when adding new displays. It defaults to the same name no matter if it exists already or not.

But I am not strongly attached to that and all your suggestions seem fine to me, with a preference for a short suffix. I actually think the X (1) thing would be nicest, but I see that it is not trivial to implement like the others. For all options one would need a recursive check for the suffixed-name's existence though.

@wjwwood
Copy link
Member

wjwwood commented Mar 22, 2016

Yeah, there's more issues with the changing of names (as @wkentaro may have discovered and finally decided not to do that). For example, nothing stops you from duplicating foo as foo', then duplicating it again. Now you have foo, foo', and another foo'. Also you can duplicate multiple items so you could duplicate foo so you also have foo', select both and duplicate again. Now you have foo, foo', foo', and foo''.

I'm thinking of going back to just duplicating it and not changing the name.

I also have a change locally that will ensure the duplicated item(s) are selected after duplication.

@wjwwood
Copy link
Member

wjwwood commented Mar 22, 2016

@wkentaro I opened wkentaro#2 against this pr.

I might do the merge myself later today.

@wjwwood wjwwood merged commit 9b99fc2 into ros-visualization:indigo-devel Mar 22, 2016
@wjwwood
Copy link
Member

wjwwood commented Mar 22, 2016

@wkentaro I merged this manually with a few changes about what is selected after duplicating and deleting displays. Thanks again!

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.

3 participants