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

Timezone change not accepted on cancel, fixes #3093 #677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Timezone change not accepted on cancel, fixes #3093 #677

wants to merge 1 commit into from

Conversation

mpdmanash
Copy link
Contributor

The issue was solved by not saving the changes on selection change
but on a 'final_apply' function that would be called by only those
options in restart menu that is supposed to save the settings ('Later'
and 'Restart Now').

@mpdmanash
Copy link
Contributor Author

Here is the preview:
datetime-cancel

@quozl
Copy link
Contributor

quozl commented Apr 11, 2016

No, this is not the correct fix. All of our My Settings control panels must make immediate and permanent changes where possible. Cancel X button is to revert the change. Check tick button is to close the panel, but if a restart is required offer to restart now, restart later, or revert the change.

Please try again. The fix will be to;

  • save the existing value at the time the control panel section is opened,
  • restore the value when "Cancel changes" is pressed.

Do you want to open a new pull request, or force push?

@mpdmanash
Copy link
Contributor Author

@quozl , Thanks for the review.

All of our My Settings control panels must make immediate and permanent changes where possible.

I was not knowing about this, hence I thought setting the settings only once and on the final step made more sense.

Nevertheless, I have made the changes. Now the initial timezone is stored at the setup and set when "cancel' is pressed. :)

I would like to force push onto this PR itself. So, if the edit is fine then I will force push for a single commit.

@quozl
Copy link
Contributor

quozl commented Apr 12, 2016

@ManashRaja, thanks. Reviewed. The variable zone in setup may be eliminated and the reference replaced with _initial_timezone.

For background information; the reasons for immediate settings commit rather than the older two-step settings commit were;

  • it was the direction taken by the GNOME project,
  • the laptop may suspend, or be powered off, at any moment.

Please review your commit message. Force push when ready.

The issue was solved by storing the initial timezone in a local variable
while loading the Control Panel section and setting this initial timezone
in the 'undo' method which is called whenever "Cancel" is pressed.
@mpdmanash
Copy link
Contributor Author

Interesting! Commit message reviewed and force pushed.

@quozl quozl added the bug label Apr 12, 2016
@quozl
Copy link
Contributor

quozl commented Apr 12, 2016

Reviewed. Looks fine. Thanks.

@mpdmanash
Copy link
Contributor Author

@samdroid-apps please check.

@samdroid-apps
Copy link
Contributor

Sorry Manash, I'm only on my phone now. I'll merege it in the coming week, when I get my laptop back on the net.

@mpdmanash
Copy link
Contributor Author

As per your convenience. :) Have a good day.

@mpdmanash
Copy link
Contributor Author

ping @samdroid-apps

@samdroid-apps
Copy link
Contributor

I tested this, but the "cancel" button stopped working for me. Log:

Calling:  set_timezone('')   # NOTE:  I added this as to the top of model.py set_timezone func
Traceback (most recent call last):
  File "/home/saam/sugar-build/build/out/install/lib/python2.7/site-packages/jarabe/controlpanel/gui.py", line 361, in __cancel_clicked_cb
    self._section_view.undo()
  File "/home/saam/sugar-build/build/out/install/share/sugar/extensions/cpsection/datetime/view.py", line 104, in undo
    self._model.set_timezone(self._initial_timezone)
  File "/home/saam/sugar-build/build/out/install/share/sugar/extensions/cpsection/datetime/model.py", line 101, in set_timezone
    raise ValueError(_('Error: timezone does not exist.'))
ValueError: Error: timezone does not exist.

@i5o
Copy link
Contributor

i5o commented Nov 15, 2016

Testing right now. I will look closely at the error that Sam reported.

@i5o
Copy link
Contributor

i5o commented Nov 15, 2016

Okay. I think that the error that Sam reported is due to not selecting a timezone before.
Maybe we should add a "try" or a conditional to check if the timezone has been set, or its using system one

@i5o
Copy link
Contributor

i5o commented Nov 15, 2016

Besides that, flake8 returns 0 errors and everything else works. Good job

@mpdmanash
Copy link
Contributor Author

@i5o Thanks for the review. Okay, I would add a conditional check.

@rhl-bthr
Copy link

rhl-bthr commented Mar 7, 2018

@ManashRaja, will you be updating the patch or may I do the same, Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants