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

Update dependencies #459

Merged
merged 12 commits into from
Mar 8, 2017
Merged

Update dependencies #459

merged 12 commits into from
Mar 8, 2017

Conversation

yuya-oc
Copy link
Contributor

@yuya-oc yuya-oc commented Mar 3, 2017

Before submitting, please confirm you've

Please provide the following information:

Summary
Update dependencies, libraries and tools.

Significant changes:

  • Electron: 1.4.13 -> 1.6.1
  • electron-builder: 10.9.2 -> 14.5.3

Issue link
#458

Test Cases

  • Installation (due to electron-builder)
  • Context menu (Electron has a change in API)

Tested on macOS 10.12 and Ubuntu 16.04 Unity

Additional Notes
https://circleci.com/gh/yuya-oc/desktop/180#artifacts

@jasonblais
Copy link
Contributor

Thanks @yuya-oc!

Did some initial testing: it looks like the desktop app UI looks a bit blurry on Windows 10. Also the server tab bar is significantly larger

image

I'll continue testing and report back any other issues, but so far looks good otherwise. No issues during installation and nothing with the context menu so far

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Mar 3, 2017

Thanks for testing. Would you write the DPI you are using?

@jasonblais
Copy link
Contributor

jasonblais commented Mar 3, 2017

Good point - here are my settings, which didn't change after downloading the above test server

image
image

This reverts mattermost#384 (f60d1fe).
Blur was a known side effect of mattermost#384 though,
the original problem looks fixed in electron v1.6.1.
@yuya-oc
Copy link
Contributor Author

yuya-oc commented Mar 3, 2017

Reproduced for me.

Size

However strangely enough, it seems that the new one is correct.

v3.6.0:
v3 6-100-125-150

https://circleci.com/gh/yuya-oc/desktop/180#artifacts
new-100-125-150

Blur

On the other hand, the blur was a known side effect of #384. So I tried reverting #384. The original problem, confused cursor looks fixed.

https://circleci.com/gh/yuya-oc/desktop/181#artifacts
blur

@jasonblais
Copy link
Contributor

I can confirm both above in Windows 10, for DPI of 100-175%, on the artifact from https://circleci.com/gh/yuya-oc/desktop/181#artifacts

So there's no issues. Interesting that the 125% level was actually always incorrect.

One thing that comes to mind (which would be separate from this PR) is whether we should reduce the size of the tabs vertically? Now that the size is correct at 125%, I feel it's taking a lot of space at the top.

@jasonblais
Copy link
Contributor

(and still no issues with installation or the context menu that I could find)

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Mar 6, 2017

In Electron, Chromium was upgraded from 53.0.2785.143 to 56.0.2924.87. As far as reading this Chromium issue, the scaling problem (bug) was fixed recently.

we should reduce the size of the tabs vertically? Now that the size is correct at 125%, I feel it's taking a lot of space at the top.

Not so bad. However to implement for the specific scaling would be hard to manage source codes in future. I think the new height should be applied for all scaling.

@jasonblais
Copy link
Contributor

Actually, if I compare at 100% screen size, there is a significant difference for me on Windows 10. Same with other screen sizes.

v3.6:
image

Artifact: https://circleci.com/gh/yuya-oc/desktop/181#artifacts

image

I still feel it's a bit blurry but I have a hard time seeing a difference from v3.6.0

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Mar 7, 2017

Ah, I agree about reducing tab height. What I wanted to mean is, something like "when 100%, 125%, 150%..." is not appropriate. They should have same height on the source code (by measuring, currently it's 42px). For example, how about using Chrome's tab height?

And then, I have found a reason why it's still a bit blurry at 125%. A webview is located at 42px height below the top of content area. Actually 42px become 52.5 physical dots on a monitor. Of course "0.5 dots" don't exist on a monitor, so Electron makes the appearance blurry.
When the tab bar doesn't exist, a webview are located at top of content area. So you would see non-blurry texts. So this would be fixed by adjusting the tab bar's height.

I feel we should take another issue to track this as you had proposed.

@jasonblais
Copy link
Contributor

Yeah, sounds good to me. Happy to create a separate issue for 3.7

If we match the height to be exactly the same as Chrome's tab height, will that also fix the blurriness (will there be an extra "0.5 dots" on a monitor?

This PR is otherwise good to merge.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Mar 8, 2017

Not same. Actually it would be a similar height compared to Chrome. The height must be an integer after multiplying the scale factor.

Thanks for many feedback!

@yuya-oc yuya-oc merged commit 0e91fba into mattermost:master Mar 8, 2017
@jasonblais
Copy link
Contributor

Sounds good, created a new issue in #462. Thanks!

yuya-oc added a commit that referenced this pull request Apr 4, 2017
Close #411 by upgrading Electron via #459
yuya-oc added a commit that referenced this pull request Apr 4, 2017
The application now respects 125% of display scaling
by upgrading Electron via #459
@yuya-oc yuya-oc mentioned this pull request Apr 29, 2017
3 tasks
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.

2 participants