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 reset-font-size #19192

Merged
merged 1 commit into from Apr 26, 2019

Conversation

Projects
None yet
3 participants
@vinkla
Copy link
Contributor

commented Apr 21, 2019

Identify the Bug

#9873

Description of the Change

If the user config is yet to be loaded oldValue equals the default font size 14px. This breaks the reset font size functionality. Fetching the font-size from the config object instead of using oldValue seems to fix the issue. I'm running:

macOS: 10.14.3
Node: v11.14.0
npm: 6.9.0

Alternate Designs

Not sure if this fix is needed at all TBH. It has been pointed out that it works fine on Windows and I know this has worked in previous versions of Atom on macOS as well. It could be an issue where the configuration hasn't yet been loaded for some reason.

Possible Drawbacks

N/A

Verification Process

  • Set the configured font size to e.g. 10
  • Increase or decrease the editor font size multiple times
  • Reset the font size window:reset-font-size
  • The font size should equal 10

Release Notes

  • Fix reset font size behaviour
Fix reset-font-size
If the user config is yet to be loaded oldValue equals the default font-size 14px. Fetching the font-size from the config object seems to fix the issue.
@vinkla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

The AppVeyor tests failed. It seems unrelated to this pull request. Right?

@vinkla vinkla marked this pull request as ready for review Apr 21, 2019

@50Wliu

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

The AppVeyor tests failed. It seems unrelated to this pull request. Right?

Yes; this is another instance of #18991.

@vinkla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@50Wliu is there something I can do on my end to rerun the tests without having to close and open the pull request? Just want to get rid of the red cross in order to get someone to look at this pull request.

@50Wliu

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I restarted it 👍

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Thanks a lot for the contribution! Codewise it looks good to me, can you add a test to verify that the behaviour does not regress in the future? 🤗

@rafeca rafeca self-assigned this Apr 26, 2019

@vinkla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Thanks for taking a look at the PR @rafeca!

I would like to add tests but I'm not really sure how I can replicate the behaviour since it now works as expected. It should be covered with this test:

atom/spec/workspace-spec.js

Lines 1729 to 1730 in cdb5ac9

workspace.resetFontSize()
expect(atom.config.get('editor.fontSize')).toBe(originalFontSize)

What is expected is that we should only save the originalFontSize property when the user updates the font size through the editor settings. Right now the oldValue parameter always returns 14 and ignores the user settings.

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Ok nevermind, let's just merge this one then :shipit:

@rafeca rafeca merged commit 41d934f into atom:master Apr 26, 2019

2 checks passed

Atom Pull Requests #20190421.4 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@vinkla vinkla deleted the vinkla:reset-font-size branch Apr 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.