Stn/unfork glutin #19120

stuartnelson3 · 2017-11-05T17:10:40Z

continuation of work started in #17311

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes do not require tests because it should be a pure refactoring.

Build is currently working on Linux and mac, need to check other platforms.

Note:

PR is currently blocked by rust-windowing/winit#349

This change is

highfive · 2017-11-05T17:10:45Z

Heads up! This PR modifies the following files:

@paulrouget: ports/glutin/lib.rs, ports/glutin/window.rs, ports/glutin/Cargo.toml, ports/servo/main.rs

highfive · 2017-11-05T17:10:47Z

Warning

These commits modify unsafe code. Please review it carefully!

paulrouget · 2017-11-06T05:30:34Z

ports/glutin/window.rs

+                },
+            } => {
+                // TODO: position is necessary according to @paulrouget, but pos is not given in
+                // the event's struct.


Yes. The PR that brings these coordinates to the wheel and click events didn't make it to upstream iirc.

paulrouget · 2017-11-06T05:32:02Z

ports/glutin/window.rs

        }
    }

    fn create_event_loop_waker(&self) -> Box<EventLoopWaker> {
        struct GlutinEventLoopWaker {
-            window_proxy: Option<glutin::WindowProxy>,
+            events_loop: Option<Arc<Mutex<glutin::EventsLoop>>>,


I don't think you need to use EventsLoop. Things will be probably easier with:

proxy: Arc<glutin::EventsLoopProxy>,

paulrouget · 2017-11-06T05:32:37Z

ports/glutin/window.rs

-                if let Some(ref window_proxy) = self.window_proxy {
-                    window_proxy.wakeup_event_loop()
+                if let Some(ref events_loop) = self.events_loop {
+                    events_loop.lock().unwrap().create_proxy().wakeup();


This will be easier if you use EventsLoopProxy. No need to create a proxy everytime.

paulrouget · 2017-11-06T05:37:36Z

An important change in upstream glutin is the way keys are handled.

If you look at servo glutin, there are 2 code paths, one for Mac+Linux, and one for Windows. Now, with upstream glutin, only the Windows code path is needed. For example, get rid of the pending_key_event_char logic.

paulrouget · 2017-11-06T05:38:31Z

For GlutinEventLoopWaker, I would recommend to copy what I did here:

https://github.com/paulrouget/servoshell/blob/13dc6fed301e9638b8fc60174f09fadd9f8fec7f/src/platform/glutin/app.rs#L21

paulrouget · 2017-11-06T05:55:43Z

About coordinates, please look at:

servo/glutin#99
servo/glutin#109

bors-servo · 2017-11-07T17:57:37Z

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

bors-servo · 2017-11-09T20:48:19Z

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

stuartnelson3 · 2017-11-11T23:09:25Z

ports/glutin/window.rs

-
-    #[cfg(target_os = "windows")]
-    fn handle_keyboard_input(&self, element_state: ElementState, _scan_code: u8, virtual_key_code: VirtualKeyCode) {
+    fn set_key(&self, element_state: ElementState, virtual_key_code: VirtualKeyCode) {


I had no idea what to name this; any suggestion is welcome.

stuartnelson3 · 2017-11-11T23:53:17Z

@paulrouget i looked at the two links you provided for getting mouse position, but I feel a bit stuck -- I'm not seeing a way in the glutin library to get anything other than the bounds of the window (inner or outer), and I couldn't see anything in the servo::gl library that would help, either.

any ideas?

paulrouget · 2017-11-12T04:54:23Z

The glutin library does not provide the mouse coordinates on click and wheel events.

So we need to fix the glutin library to do so. Now, glutin is split in 2: glutin for the opengl stuff, and winit for the windowing and events. So what you actually need to fix is winit.

The 2 links show how we did it for the servoglutin.

So someone need to adapt these 2 PRs to the winit library.

bors-servo · 2017-11-13T16:21:29Z

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

bors-servo · 2017-11-14T19:30:11Z

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

bors-servo · 2017-11-16T03:58:52Z

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

stuartnelson3 · 2017-12-05T17:16:49Z

Get the icon loading patches either upstreamed or rebased on a new glutin fork.
Get the platform_window() code working [or drop CEF for now?].

@jdm successfully got glutin bumped. The last steps are the above two, I believe. I'm not sure what how folks feel about completing the above two

stuartnelson3 · 2017-12-06T09:05:00Z

Hm, the build is failing on ./mach test-tidy --no-progress --all, not sure what to do about the duplicate packages it's complaining about.

Eijebong · 2017-12-06T09:10:10Z

@stuartnelson3 This patch needs to be upstreamed I think servo/glutin@0851887. I would do it myself but don't have any osx to test it. Let's ping @nox about it :)

bors-servo · 2017-12-07T07:59:28Z

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


        Borrow checker stuff to figure out


        Mutex seems to solve the issue

But I'm not sure if this will produce the desired behavior.


        Remove NestedEventLoopListener code

rustfmt for free.


        Use proxy in event loop waker


        Address some compiler warnings


        fix rebase


        Remove macos/linux branched key handling


        update code for getting window size


        Update to glutin v0.11

bors-servo · 2017-12-08T14:52:51Z

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

paulrouget · 2018-01-16T08:06:29Z

@stuartnelson3 can you describe what's left to be done here? Do you need an early review?

paulrouget · 2018-01-16T08:16:58Z

Also, I'd be happy to take over if you don't have time to continue.

stuartnelson3 · 2018-01-16T08:56:24Z

@paulrouget yeah feel free to take over. the patch in this comment needs to be upstreamed: #19120 (comment)

Good luck!

jdm · 2018-01-16T15:15:17Z

@paulrouget I suspect we're still missing fixes for the work items listed in the original comment in #17311.

paulrouget · 2018-01-29T13:19:42Z

Taking over this PR. See #19895

@stuartnelson3 thanks a lot for your initial PR. That made my life a lot easier :)


        Auto merge of #19895 - paulrouget:unfork, r=paulrouget

unfork glutin I'm taking over #19120. Fix #18918. Beside the change in the API, I had to rewrite main loop. At this point, we should have a very similar behavior as servo-glutin.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19895)

highfive assigned jdm Nov 5, 2017

highfive added the S-awaiting-review label Nov 5, 2017

paulrouget reviewed Nov 6, 2017

View changes

highfive added the S-needs-rebase label Nov 7, 2017

stuartnelson3 force-pushed the stuartnelson3:stn/unfork-glutin branch from 53f3af4 to c10d77c Nov 7, 2017

highfive added the S-needs-rebase label Nov 9, 2017

stuartnelson3 force-pushed the stuartnelson3:stn/unfork-glutin branch from c10d77c to 9ce14f4 Nov 11, 2017

stuartnelson3 reviewed Nov 11, 2017

View changes

stuartnelson3 mentioned this pull request Nov 12, 2017

Expose mouse pos rust-windowing/winit#348

Closed

highfive added the S-needs-rebase label Nov 13, 2017

stuartnelson3 force-pushed the stuartnelson3:stn/unfork-glutin branch from 49b420b to 7442d4e Nov 14, 2017

highfive added the S-needs-rebase label Nov 14, 2017

stuartnelson3 force-pushed the stuartnelson3:stn/unfork-glutin branch from 7442d4e to 9b052a6 Nov 15, 2017

stuartnelson3 mentioned this pull request Nov 15, 2017

Send mouse position after focused/cursorenter events rust-windowing/winit#349

Merged

highfive added the S-needs-rebase label Nov 16, 2017

jdm added the S-blocked-on-external label Nov 16, 2017

highfive added the S-needs-rebase label Nov 29, 2017

stuartnelson3 force-pushed the stuartnelson3:stn/unfork-glutin branch 2 times, most recently from b369eda to d82ad5a Dec 4, 2017

jdm removed the S-needs-rebase label Dec 5, 2017

highfive added the S-needs-rebase label Dec 7, 2017

stuartnelson3 added 9 commits Nov 4, 2017

Borrow checker stuff to figure out

edd1f72

Mutex seems to solve the issue

45d47c2

But I'm not sure if this will produce the desired behavior.

Remove NestedEventLoopListener code

b9ede5d

rustfmt for free.

Use proxy in event loop waker

e8ac550

Address some compiler warnings

673d2c8

fix rebase

2cb00af

Remove macos/linux branched key handling

d4c1f1c

update code for getting window size

6860481

Update to glutin v0.11

Loading status checks…

601663b

stuartnelson3 force-pushed the stuartnelson3:stn/unfork-glutin branch from dd07964 to 601663b Dec 7, 2017

highfive added the S-needs-rebase label Dec 8, 2017

emilio mentioned this pull request Dec 8, 2017

Wrong keycodes reported for non-standard layouts. Fixed upstream servo/glutin#139

Closed

paulrouget mentioned this pull request Jan 29, 2018

unfork glutin #19895

Merged

paulrouget closed this Jan 29, 2018

servo / servo

Stn/unfork glutin #19120

Stn/unfork glutin #19120

stuartnelson3 commented Nov 5, 2017 •

edited

highfive commented Nov 5, 2017

highfive commented Nov 5, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget commented Nov 6, 2017

paulrouget commented Nov 6, 2017

paulrouget commented Nov 6, 2017

bors-servo commented Nov 7, 2017

bors-servo commented Nov 9, 2017

This comment has been minimized.

stuartnelson3 commented Nov 11, 2017

paulrouget commented Nov 12, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 14, 2017

bors-servo commented Nov 16, 2017

stuartnelson3 commented Dec 5, 2017

stuartnelson3 commented Dec 6, 2017

Eijebong commented Dec 6, 2017

bors-servo commented Dec 7, 2017

bors-servo commented Dec 8, 2017

paulrouget commented Jan 16, 2018

paulrouget commented Jan 16, 2018

stuartnelson3 commented Jan 16, 2018

jdm commented Jan 16, 2018

paulrouget commented Jan 29, 2018

servo / servo

Join GitHub today

Stn/unfork glutin #19120

Stn/unfork glutin #19120

Conversation

stuartnelson3 commented Nov 5, 2017 • edited

highfive commented Nov 5, 2017

highfive commented Nov 5, 2017

This comment has been minimized.

paulrouget Nov 6, 2017

This comment has been minimized.

paulrouget Nov 6, 2017

This comment has been minimized.

paulrouget Nov 6, 2017

paulrouget commented Nov 6, 2017

paulrouget commented Nov 6, 2017

paulrouget commented Nov 6, 2017

bors-servo commented Nov 7, 2017

bors-servo commented Nov 9, 2017

This comment has been minimized.

stuartnelson3 Nov 11, 2017

stuartnelson3 commented Nov 11, 2017

paulrouget commented Nov 12, 2017

bors-servo commented Nov 13, 2017

bors-servo commented Nov 14, 2017

bors-servo commented Nov 16, 2017

stuartnelson3 commented Dec 5, 2017

stuartnelson3 commented Dec 6, 2017

Eijebong commented Dec 6, 2017

bors-servo commented Dec 7, 2017

bors-servo commented Dec 8, 2017

paulrouget commented Jan 16, 2018

paulrouget commented Jan 16, 2018

stuartnelson3 commented Jan 16, 2018

jdm commented Jan 16, 2018

paulrouget commented Jan 29, 2018

stuartnelson3 commented Nov 5, 2017 •

edited