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

Add Linux icon #11

Merged
merged 1 commit into from
Dec 17, 2017
Merged

Add Linux icon #11

merged 1 commit into from
Dec 17, 2017

Conversation

picandocodigo
Copy link

@picandocodigo picandocodigo commented Dec 15, 2017

An initial attempt that fixes #8. Not sure if the paths and the modifications in the build script are ok. But as you can see, this makes it work with yarn start in the alt-tab menu:
alt-tab

And in the activities panel:
activities

Let me know if anything needs to be modified or improved. Thanks!

@picandocodigo picandocodigo changed the title Add Linux icon. Fixes #8 Add Linux icon Dec 15, 2017
Copy link
Owner

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

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

Just one suggestion about the packaging side, everything else looks 💎

script/build.ts Outdated
@@ -178,6 +181,12 @@ function copyStaticResources() {
fs.copySync(common, destination, { clobber: false })
}

function copyLogo() {
const logoImage = path.join(projectRoot, 'app', 'static', 'logos', '512x512.png')
Copy link
Owner

Choose a reason for hiding this comment

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

We actually have a folder of dedicated Linux content at app/static/linux/ that already gets copied across.

For the sake of simplicity I'd just copy this file into that location with the name you want it to be, rather than have a separate method that runs on all platforms.

@shiftkey
Copy link
Owner

Oh, and run yarn lint:fix on your branch to address the coding style issues picked up by TravisCI.

@shiftkey shiftkey changed the base branch from master to linux December 15, 2017 01:23
@picandocodigo picandocodigo force-pushed the linux_icon branch 2 times, most recently from 7d169fc to f696118 Compare December 15, 2017 10:53
@picandocodigo
Copy link
Author

@shiftkey updated the PR, moved the icon to the linux static directory and using that instead. Thanks!

@@ -59,6 +60,10 @@ export class AppWindow {
windowOptions.frame = false
}

if (__LINUX__) {
windowOptions.icon = path.join(__dirname, 'icon-logo.png')
Copy link
Owner

Choose a reason for hiding this comment

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

I think you had this right previously with looking in the static subdirectory, like this:

windowOptions.icon = path.join(__dirname, 'static', 'icon-logo.png')

Here's what the app directory looks like after building:

$ pwd
/home/parallels/src/desktop/dist/desktop-linux-x64/resources/app
$ ls
ask-pass.js      crash.html    highlighter.js      node_modules     ui.css
ask-pass.js.map  crash.js      highlighter.js.map  package.json     ui.css.map
cli.js           crash.js.map  index.html          renderer.js      yarn.lock
cli.js.map       emoji         keytar.node         renderer.js.map
crash.css        emoji.json    main.js             runas.node
crash.css.map    git           main.js.map         static
$ ls static
ask-pass-trampoline.sh      empty-no-repo.svg
choosealicense.com          gitignore
default-avatar.png          icon-logo.png
empty-no-branches.svg       licenses.json
empty-no-commit.svg         welcome-illustration-left-bottom.svg
empty-no-file-selected.svg  welcome-illustration-left-top.svg
empty-no-pull-requests.svg  welcome-illustration-right.svg

But on my Ubuntu 16.04 VM I got a generic icon when running the app in development mode:

Same thing with the installed AppImage:

Maybe there's something off with my setup?

Copy link
Author

Choose a reason for hiding this comment

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

@shiftkey that's odd. When running yarn dev:build on my machine, a icon-logo.png file is being copied to dist/desktop-linux-x64/resources/app/. Are you building it a different way? I'm not that familiar with yarn yet, so there might be a different release task.

Anyway, changing it back to static/icon-logo.png works too, I've updated the PR.

Copy link
Owner

Choose a reason for hiding this comment

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

@picandocodigo I'm only doing yarn build:dev or yarn build:prod

Copy link
Author

Choose a reason for hiding this comment

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

Just tried yarn build:prod too, and in both cases (prod and dev) for some reason the logo is being copied to both /dist/desktop-linux-x64/resources/app and /dist/desktop-linux-x64/resources/app/static.

If it doesn't happen in your VM, it might be an issue with my setup or something. The icon still shows up using windowOptions.icon = path.join(__dirname, 'static', 'icon-logo.png') . And I rm -rf the dist directory before every yarn build.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this might have been some caching in the out directory to blame. We're not as aggressive with cleaning that directory up between builds - we should be nuking dist.

@shiftkey
Copy link
Owner

I'll take this in an prepare another release candidate sometime this week to see if it's addressed.

Thanks for the work @picandocodigo!

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.

App Icon is missing in some places in GNOME
2 participants