-
Notifications
You must be signed in to change notification settings - Fork 12
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
show copy button for all single fields #21
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, but I have some questions.
item_view.h
Outdated
} | ||
|
||
void processSingleField(std::string label, std::string value, bool conceal) { | ||
void processSingleField(std::string label, std::string value, bool conceal, bool copy_button) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this always true? If so, let's not add another parameter.
item_view.h
Outdated
@@ -81,9 +81,9 @@ class ItemView : public Gtk::Grid { | |||
|
|||
if (conceal && !isTOTP) | |||
value_widget->set_visibility(false); | |||
attach(*value_widget, 1, my_index, conceal ? 1 : 3, 1); | |||
attach(*value_widget, 1, my_index, conceal || copy_button ? 1 : 3, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this becomes attach(*value_widget, 1, my_index, conceal ? 1 : 2, 1);
item_view.h
Outdated
|
||
if (conceal) { | ||
if (copy_button) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, since this is just always true, I don't think it needs to be in a conditional
ae3b948
to
91bf4b9
Compare
I found myself selecting and copying username fields manually. So I figured having a copy button on all single text fields could be useful.
I guess it's a bit of an opinion thing whether or not this will merge. The copy_button param of processSingleField decides if the button is added or not, but is hard-coded to
true
. Maybe someday user preferences will control it.