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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

294 reverse phrasing #299

Merged
merged 9 commits into from Nov 16, 2018
Merged

294 reverse phrasing #299

merged 9 commits into from Nov 16, 2018

Conversation

@ls42
Copy link
Contributor

@ls42 ls42 commented Oct 8, 2018

Fixes #294

This takes care of

  • Rephrasing to a more positive version
  • Tick CheckButton by default
  • Add translation string to po files

Tested it, worked for me 馃槂

ls42 added 2 commits Oct 8, 2018
@@ -70,6 +70,8 @@ public class PantheonTerminal.UnsafePasteDialog : Gtk.Dialog {
add_action_widget (cancel_button, 1);
add_action_widget (ignore_button, 0);

show_protection_warnings.set_active (true);

This comment has been minimized.

@cassidyjames

cassidyjames Oct 8, 2018
Member

If this is bound to the setting, does it need to be explicitly marked as active?

This comment has been minimized.

@cassidyjames

cassidyjames Oct 8, 2018
Member

I tested locally and it appears to still properly check itself when bound to the setting, so I believe this should be removed.

This comment has been minimized.

@ls42

ls42 Oct 8, 2018
Author Contributor

I added it so the dialog still has the same default behaviour: Showing the dialog if the CheckButton is untouched by the user. The old phrasing requires the user to act to mute the dialog. The new phrasing should still require the user to act.

I think this is required to fulfill the requirement "and have it checked by default." that your made in the issues description.

Sorry in advance if maybe I don't get what your mean.

This comment has been minimized.

@cassidyjames

cassidyjames Oct 8, 2018
Member

@ls42 No worries, it's kind of confusing! But by using a binding to the setting, the check will automatically check itself if that underlying setting is active. The setting is already active by default (controlled by the default GSchema), and it must be active to show the dialog in the first place, so this line is just unnecessary.

This comment has been minimized.

@ls42

ls42 Oct 8, 2018
Author Contributor

Alright! Very interesting. I remove the line and it's still working.

Copy link
Member

@cassidyjames cassidyjames left a comment

Code looks good, thanks for the contribution!

I think lastly we should drop the translation changes from this PR to prevent merge conflicts with master. It also makes this diff much easier for other contributors to review. Once this is merged, we can push the updated translations.

ls42 added 2 commits Oct 9, 2018
This reverts commit 6907624.
@ls42
Copy link
Contributor Author

@ls42 ls42 commented Oct 9, 2018

I removed the changes to the po files. I'm not sure why they made the CI fail, though.

@cassidyjames
Copy link
Member

@cassidyjames cassidyjames commented Oct 9, 2018

@ls42 it looks like CI is failing in master right now, so it's not your branch that's doing it.

Copy link
Member

@cassidyjames cassidyjames left a comment

Looks good! Thanks for the contribution. @codygarver is this safe to merge with a string change since there was a recent release?

@ls42
Copy link
Contributor Author

@ls42 ls42 commented Oct 9, 2018

Thanks for the contribution

You're very welcome. I hope I can get involved even more, I really enjoy using elementaryOS and hope it really takes off (number-of-happy-users-wise)!

cassidyjames and others added 3 commits Oct 10, 2018
@danrabbit danrabbit merged commit 8fb201b into elementary:master Nov 16, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants