Use a winres to give servo.exe an icon on Windows #11969

Coder206 · 2016-06-30T20:01:44Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11315 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-06-30T20:01:50Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @wafflespeanut (or someone else) soon.

waddlesplash · 2016-06-30T20:18:19Z

Cc @metajack - this might help with adding a manifest via a resource file

Coder206 · 2016-07-02T00:00:48Z

@waddlesplash Do you know why this code failed Travis?

waddlesplash · 2016-07-02T00:02:45Z

Yeah, it says so:

./components/servo/build.rs:78: no newline at EOF

./resources/windows.rc:1: incorrect license

waddlesplash · 2016-07-02T00:03:04Z

components/servo/build.rs

+fn windows_main() {
+        let out_dir = env::var("OUT_DIR").ok().expect("can't find out_dir");
+
+    Command::new("windres").args(&["src/windows.rc",  "-o"])


Shouldn't this be resources/ not src/?

waddlesplash · 2016-07-02T00:03:25Z

resources/windows.rc

+        MENUITEM "&New\tCtrl+N", MENU_NEW
+        MENUITEM "E&xit", MENU_EXIT
+    }
+}


All this MENU stuff goes; only the MANIFEST and ICON should stay.

Coder206 · 2016-07-02T00:04:35Z

@waddlesplash Thanks, I will look into this.

waddlesplash · 2016-07-02T00:04:38Z

Also ideally this should probably be combined with #11967 (as that one has the manifest & icon and this one does not)

Coder206 · 2016-07-02T00:05:56Z

@waddlesplash I think it would be good to update #12125 also in that case

waddlesplash · 2016-07-02T00:10:47Z

erm... @metajack needs the branch so he can incorporate that code, no?

Coder206 · 2016-07-02T00:12:19Z

@waddlesplash Do I rework the code?

Coder206 · 2016-07-08T12:51:13Z

I am not sure why this second one failed...

Error message is about python.

======================================================================

FAIL: test_file_list (servo_tidy_tests.test_tidy.CheckTidiness)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/servo/servo/python/tidy/servo_tidy_tests/test_tidy.py", line 128, in test_file_list

    self.assertEqual([os.path.join(base_path, 'whee', 'test.rs')], lst)

AssertionError: Lists differ: ['./python/tidy/servo_tidy_tes... != ['./python/tidy/servo_tidy_tes...

Second list contains 1 additional elements.

First extra element 1:

./python/tidy/servo_tidy_tests/test_ignored/whee/foo/bar.rs

- ['./python/tidy/servo_tidy_tests/test_ignored/whee/test.rs']

?                                                            ^

+ ['./python/tidy/servo_tidy_tests/test_ignored/whee/test.rs',

?                                                            ^

+  './python/tidy/servo_tidy_tests/test_ignored/whee/foo/bar.rs']

----------------------------------------------------------------------

Ran 12 tests in 0.030s

FAILED (failures=1)

KiChjang · 2016-07-08T12:53:27Z

Try rebasing against master.

KiChjang · 2016-07-08T13:57:38Z

$ ./mach test-tidy --no-progress --all

Checking files for tidiness...

./components/servo/build.rs:78: no newline at EOF

./resources/windows.rc:1: incorrect license

Coder206 · 2016-07-08T14:27:54Z

@KiChjang I need to use another editor to fix the first tidy error and for the second: I have no idea what I need to do, I didn't think I had a license in the file. Thanks for posting.

jdm · 2016-07-08T15:00:30Z

You can add the rc file to the list of ignored files in tidy.py.

Coder206 · 2016-07-08T15:01:48Z

@jdm Thanks

larsbergstrom · 2016-07-11T15:05:13Z

I think this is ready to go, but it needs a cherry-pick / rebase on top of the servo/servo master branch to remove all of the merge commits that have snuck in there. Let us know if you need some help with how to do that! Basically, the idea is that you need to create a new branch based on the master branch from servo/servo, use git cherry-pick to pick up the actual commits you want to keep, then git push -f that branch you just created over the one that's in use in this PR right now.

nox · 2017-04-03T07:40:29Z

@Coder206 Ping?

Coder206 · 2017-04-07T15:39:04Z

I am having trouble getting Servo to compile with Windows.
I tried using VS 2017 with build 14.0 and I got Rust compiler errors and it tried to install Rust but also failed, the link doesn't exist...

Error in script usage. The correct usage is:
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" [option]
  or
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" [option] store
  or
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" [option] [version number]
  or
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" [option] store [version number]
where [option] is: x86 | amd64 | arm | x86_amd64 | x86_arm | amd64_x86 | amd64_arm
where [version number] is either the full Windows 10 SDK version number or "8.1" to use the windows 8.1 SDK
:
The store parameter sets environment variables to support
  store (rather than desktop) development.
:
For example:
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" x86_amd64
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" x86_arm store
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" x86_amd64 10.0.10240.0
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" x86_arm store 10.0.10240.0
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" x64 8.1
    "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\..\..\VC\vcvarsall.bat" x64 store 8.1
:
Please make sure either Visual Studio or C++ Build SKU is installed.
looking for rustc at C:\Users\Coder\Documents\servo\.servo\rust\474f7a91eec8cba83b7eb7a578a7adb70614f877\rustc-nightly-x86_64-unknown\rustc\bin\rustc.exe
Downloading Rust compiler...
Download failed (404): Not Found - https://s3.amazonaws.com/rust-lang-ci/rustc-builds/474f7a91eec8cba83b7eb7a578a7adb70614f877/rustc-nightly-x86_64-unknown.tar.gz

I am not sure where to go from here....

Any help is greatly appreciated! :)

atouchet · 2017-04-10T20:53:13Z

@Coder206 have you taken a look at #16307?

Coder206 · 2017-04-10T22:17:57Z

Many thanks @atouchet :)


        No success code

Coder206 · 2017-04-11T01:21:23Z

The code I pushed a moment ago is simply to provide an update to everyone on the status of the PR, I am able to compile but I am missing something to get the icon to show. Help and comments are greatly appreciated :)

UK992 · 2017-04-11T01:28:45Z

ports/servo/build.rs

+
+fn windows_main() {
+    let mut res = winres::WindowsResource::new();
+    res.set_resource_file("windows.rc");


res.set_icon("../../resources/Servo.ico") should make it work.

@UK992 Thanks for the suggestion, it would simplify the code a lot! :) However, only a corrupted version of the icon shows up in my taskbar...

Do you think this is a problem with winres?

@Coder206 I have experienced similar icon corruption issues without this PR applied (see #16326). I haven't been able to figure out the cause yet.

When working on the code last night, I noticed there is a conflicting file setting the icon, it's support/windows/Servo.wxs.mako. After removing it, I did not encounter any corruption in the icon but the application icon on it's window is still missing like the above.

UPDATE: The above is true however, the icon is corrupted when I am using the winres crate.

Coder206 · 2017-04-11T19:28:17Z

I have been working on the code today with no success. What is creating the application window in Windows? It appears that there is some code in addition to support/windows/Servo.wxs.mako's two lines that specify the .ico. Is there a concept I am missing here, help is greatly appreciated.

Here is the debug build:

Notice the Command Prompt with the icon ^

Here is the release build:

retep998 · 2017-04-11T20:31:03Z

The icon specified in the resource file is used as the icon for the exe in windows explorer. To specify an icon for the window itself you have to load the icon and specify it in the window class that is registered and then used by the window.

Coder206 · 2017-04-11T20:57:26Z

@retep998 It appears that /ports/glutin/windows.rs is taking care of this but after changing the icon path to servo.ico in the same directory, Servo compiles but no icon in the title bar. Any ideas as to why this is happening?

retep998 · 2017-04-11T21:03:52Z

@Coder206 I can't find where with_icon is defined so I can't see whether it is actually doing the correct thing.

Coder206 · 2017-04-11T21:17:40Z

@retep998 That's strange, the code seems to indicate that it's from glutin::WindowBuilder but there is no method with_icon in the official repository and in Servo's fork. That line of code is the only result when searching with_icon in servo/servo and its glutin fork (no results).

UK992 · 2017-04-11T21:53:45Z

Window icon on Windows is not implemented, there was an attempt on upstream repo (rust-windowing/glutin#688), but it's inactive for long time.

bors-servo · 2017-05-25T14:02:25Z

☔ The latest upstream changes (presumably #17031) made this pull request unmergeable. Please resolve the merge conflicts.

nox · 2017-09-24T12:54:43Z

@Coder206 Are you still working on this?

Coder206 · 2017-09-24T13:16:27Z

@nox No, I am not actively working on this. I was having trouble getting winres to show the icon.

nox · 2017-09-24T13:33:18Z

Ok, closing for now. Thanks for trying anyway! I hope we'll get to have our fancy doge icon one day. :)


        Auto merge of #20316 - UK992:win32-icon, r=jdm

Windows: Add icon to servo.exe Based on #11969 --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20316)


        servo: Merge #20316 - Windows: Add icon to servo.exe (from UK992:win3…

…2-icon); r=jdm Based on servo/servo#11969 --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors Source-Repo: https://github.com/servo/servo Source-Revision: 840c44e2a8db05c11d5c87a7ce644001732b0cfa --HG-- rename : servo/components/servo/servo.exe.manifest => servo/ports/servo/platform/windows/servo.exe.manifest extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 595aeb4fc84adb07427d24b960e92b8bee6b7507


        servo: Merge #20316 - Windows: Add icon to servo.exe (from UK992:win3…

…2-icon); r=jdm Based on servo/servo#11969 --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors Source-Repo: https://github.com/servo/servo Source-Revision: 840c44e2a8db05c11d5c87a7ce644001732b0cfa UltraBlame original commit: e1d92ece90afece3b0151123135a1bbe7ad020b6


        servo: Merge #20316 - Windows: Add icon to servo.exe (from UK992:win3…

…2-icon); r=jdm Based on servo/servo#11969 --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors Source-Repo: https://github.com/servo/servo Source-Revision: 840c44e2a8db05c11d5c87a7ce644001732b0cfa UltraBlame original commit: e1d92ece90afece3b0151123135a1bbe7ad020b6


        servo: Merge #20316 - Windows: Add icon to servo.exe (from UK992:win3…

…2-icon); r=jdm Based on servo/servo#11969 --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors Source-Repo: https://github.com/servo/servo Source-Revision: 840c44e2a8db05c11d5c87a7ce644001732b0cfa UltraBlame original commit: e1d92ece90afece3b0151123135a1bbe7ad020b6

highfive assigned wafflespeanut Jun 30, 2016

highfive added the S-awaiting-review label Jun 30, 2016

Coder206 changed the title ~~For waddle_desktop~~ Use a winres to give servo.exe an icon on Windows Jun 30, 2016

larsbergstrom mentioned this pull request Jul 1, 2016

Windows Nightly blockers #12125

Closed

11 of 11 tasks complete

waddlesplash reviewed Jul 2, 2016
View changes

resources/windows.rc Outdated

MENUITEM "&New\tCtrl+N", MENU_NEW

MENUITEM "E&xit", MENU_EXIT

}

}

This comment has been minimized.

Sign in to view

waddlesplash Jul 2, 2016

All this MENU stuff goes; only the MANIFEST and ICON should stay.

Coder206 closed this Jul 2, 2016

Coder206 deleted the Coder206:icon branch Jul 2, 2016

Coder206 restored the Coder206:icon branch Jul 2, 2016

Coder206 reopened this Jul 8, 2016

Coder206 mentioned this pull request Jul 8, 2016

Add icon to Servo on Windows #11315

Closed

KiChjang added S-fails-tidy and removed S-awaiting-review labels Jul 8, 2016

highfive added the S-awaiting-review label Jul 9, 2016

nox closed this Mar 24, 2017

nox reopened this Mar 24, 2017

atouchet mentioned this pull request Apr 10, 2017

Problems with Servo icon in Windows taskbar #16326

Closed

No success code

Loading status checks…

00a1b4f

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 11, 2017

UK992 reviewed Apr 11, 2017

View changes

highfive added the S-needs-rebase label May 25, 2017

nox closed this Sep 24, 2017

UK992 mentioned this pull request Mar 16, 2018

Windows: Add icon to servo.exe #20316

Merged

2 of 2 tasks complete

servo / servo

Join GitHub today

Use a winres to give servo.exe an icon on Windows #11969

Use a winres to give servo.exe an icon on Windows #11969

Conversation

Coder206 commented Jun 30, 2016 • edited

highfive commented Jun 30, 2016

waddlesplash commented Jun 30, 2016

Coder206 commented Jul 2, 2016

waddlesplash commented Jul 2, 2016

This comment has been minimized.

waddlesplash Jul 2, 2016

This comment has been minimized.

waddlesplash Jul 2, 2016

Coder206 commented Jul 2, 2016

waddlesplash commented Jul 2, 2016 • edited

Coder206 commented Jul 2, 2016

waddlesplash commented Jul 2, 2016

Coder206 commented Jul 2, 2016

Coder206 commented Jul 8, 2016

KiChjang commented Jul 8, 2016

KiChjang commented Jul 8, 2016

Coder206 commented Jul 8, 2016

jdm commented Jul 8, 2016

Coder206 commented Jul 8, 2016

larsbergstrom commented Jul 11, 2016

nox commented Apr 3, 2017

Coder206 commented Apr 7, 2017 • edited

atouchet commented Apr 10, 2017

Coder206 commented Apr 10, 2017

Coder206 commented Apr 11, 2017

This comment has been minimized.

UK992 Apr 11, 2017

This comment has been minimized.

Coder206 Apr 11, 2017 • edited

This comment has been minimized.

atouchet Apr 11, 2017 • edited

This comment has been minimized.

Coder206 Apr 11, 2017 • edited

Coder206 commented Apr 11, 2017 • edited

retep998 commented Apr 11, 2017

Coder206 commented Apr 11, 2017 • edited

retep998 commented Apr 11, 2017

Coder206 commented Apr 11, 2017

UK992 commented Apr 11, 2017 • edited

bors-servo commented May 25, 2017

nox commented Sep 24, 2017

Coder206 commented Sep 24, 2017

nox commented Sep 24, 2017

Coder206 commented Jun 30, 2016 •

edited

waddlesplash commented Jul 2, 2016 •

edited

Coder206 commented Apr 7, 2017 •

edited

Coder206 Apr 11, 2017 •

edited

atouchet Apr 11, 2017 •

edited

Coder206 Apr 11, 2017 •

edited

Coder206 commented Apr 11, 2017 •

edited

Coder206 commented Apr 11, 2017 •

edited

UK992 commented Apr 11, 2017 •

edited