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 #4733 - remove warning on username change #257

Closed

Conversation

ana-balica
Copy link
Contributor

If the user changes the username on My Settings -> About me view and then reverts it to the initial username, remove the warning about restart.

If the user changes the username on My Setting -> About me view and then reverts it to the initial username, remove the warning about restart.
self._nick_alert.hide()
else:
self._nick_alert.props.msg = self.restart_msg
self.needs_restart = True
Copy link
Contributor

Choose a reason for hiding this comment

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

It may need to restart if either the nick or the color have changed, so we can't set needs_restart to True.

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 else block means that the nick have changed and it's different than the original one. If we don't set self.needs.restart = True, then on saving the changes, the user doesn't get a notification about restart.

To my understanding self.needs_restart is a class property that can be set to True either on nick change or color change. In this case we want to notify that nick has changed and user needs a restart.

If I misunderstood your suggestion, I will appreciate extra explanation to fix the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was looking at the other case, when self.needs_restart = False . What if...

  • user changes color -> needs_restart is set to True
  • user changes nick -> needs_restart is set to True
  • user changes nick back to original -> needs_restart is set to False

The dialog will not require restart, even when the color has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you are absolutely right.
Made the fix and committed it.

What is the workflow now? Should I cancel the current pull-request?
Is the rebase required to squash 2 commit together and push them to a new branch?
I am reading this guide - http://developer.sugarlabs.org/contributing.md.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, you merge locally in another branch, name it something like "fix_namechange_try2" and push the new branch to github. I'll close this one now. Thanks!

@manuq
Copy link
Contributor

manuq commented Feb 25, 2014

Thanks for the patch, @ana-balica

Here are two testcases that I think should pass. With your patch, the first one passes, but not the second.

Testcase 1

  • change the nick
  • wait until the alert below the text entry appears
  • change back the nick to the original
  • wait until the alert dissapears
  • press the accept icon at the top left of the dialog

The section should close without asking for a restart. This works with your patch.

Testcase 2

  • change the nick
  • press the accept icon at the top left of the dialog
  • a "warning, needs restart" alert appears at the top
  • press the "cancel changes" button
  • change back the nick to the original
  • press the accept icon at the top again

The "warning, needs restart" still alert appears. It should not appear because the nick is the same as the original.

I think testcase 1 describes the ticket #4733. So if you change the restart flag issue, I'm ok to push.

If you also want to fix testcase 2, great! I think you need to remove the alert and the restart flag when the

        if widget.get_text() == self._model.get_nick():
+            self.needs_restart = False
+            self.restart_alerts.remove('nick')
+            self._nick_alert.hide()
            return False

Note this has the same issue in the restart flag, so is just an example.

@ana-balica
Copy link
Contributor Author

I have reproduced testcase 2 successfully and the issue is clear to me.
After fixing issue that describes testcase 1, I will take a look at it.

Thank you @manuq for such an explicit description.

@manuq
Copy link
Contributor

manuq commented Feb 25, 2014

Great @ana-balica, I'm fine with either separate commits, or only one. Whatever works better for you.

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.

None yet

2 participants