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

COMPASS-587: Improve the about Compass dialog #733

Merged
merged 4 commits into from Jan 9, 2017
Merged

Conversation

pzrq
Copy link
Contributor

@pzrq pzrq commented Jan 5, 2017

The about dialog now looks correct and polished on all of our supported platforms. Two Linux compatibility issues have been fixed: the about dialog now shows the Compass icon instead of the system default and is now closable:

ubuntu

This also resolves a very long-standing annoyance reported by our Windows users way back in 2015.

windows 10

Fixes COMPASS-587, COMPASS-352

@pzrq
Copy link
Contributor Author

pzrq commented Jan 6, 2017

@durran If you could review this it would be appreciated.

Please let me know if you have any specific feedback, e.g. if I should do the Windows testing and post screenshots, also happy to let COMPASS-586 go through first and rebase accordingly if you've started on that.

@pzrq
Copy link
Contributor Author

pzrq commented Jan 6, 2017

I finally figured out how to submit a patch to Evergreen via evergreen patch --project=10gen-compass-master --yes --finalize -v all -t all to see if I could get a Windows build to test, but got the error:

error: cannot apply binary patch to 'src/app/images/compass-dialog-icon.png' without full index line

https://evergreen.mongodb.com/task/10gen_compass_master_windows_64_compile_6eac6326bec13a93d9556b62c7ebfebedbac15a5_17_01_06_07_25_08

I guess the next step to try next week if this isn't yet merged will be evergreen patch-file ... with: http://stackoverflow.com/questions/17152171/git-cannot-apply-binary-patch-without-full-index-line

@rueckstiess
Copy link
Member

@pzrq ask me about this again on Monday. patch-file is correct.

@imlucas imlucas requested review from imlucas and removed request for durran January 6, 2017 20:26
@imlucas imlucas self-assigned this Jan 6, 2017
@durran durran self-requested a review January 6, 2017 21:08
@durran durran self-assigned this Jan 6, 2017
Should not change Windows, should fix Red Hat, should not affect macOS (i.e. darwin, see src/main/menu.js which does not add this menu option for darwin):
https://github.com/electron/electron/blob/master/docs/api/dialog.md#dialogshowmessageboxbrowserwindow-options-callback
So it looks nicer on Windows and Linux.
As COMPASS 352 indicates Windows does funky things if we don’t add one.
@pzrq
Copy link
Contributor Author

pzrq commented Jan 9, 2017

patch-file did not work for me, what did from the Evergreen public wiki was:

evergreen patch --project=10gen-compass-master --yes --finalize -v all -t all \
-- --binary --full-index

Next step is to test in Windows VMs, then I'll update the initial post.
https://evergreen.mongodb.com/task/10gen_compass_master_windows_64_compile_ca10016a97d4106b8ed109569aabdb364df95a41_17_01_09_01_16_44

@pzrq
Copy link
Contributor Author

pzrq commented Jan 9, 2017

Setting this back to Work in progress as the icon error from Red Hat 7 (non development box) also affected Windows 10. e.g. perhaps path must be constructed via path.join.

Of interest, suggesting it should work with a string path or NativeImage: electron/electron#4073

windows 10 about box

@pzrq
Copy link
Contributor Author

pzrq commented Jan 9, 2017

OK I am now satisfied that the latest commit Windows/RedHat via Evergreen: Reuse RESOURCES path resolves the path issue on all supported platforms, setting to ready for review.

Copy link
Contributor

@imlucas imlucas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @pzrq

@imlucas imlucas changed the title COMPASS-587 About Compass Dialog COMPASS-587: Improve the about Compass dialog Jan 9, 2017
@imlucas imlucas merged commit e65298b into master Jan 9, 2017
@imlucas imlucas deleted the COMPASS-587-menu branch January 9, 2017 20:09
@pzrq
Copy link
Contributor Author

pzrq commented Jan 10, 2017

@imlucas 👍

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.

None yet

4 participants