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

feat(native): add Gtk APIs for windows/widgets #982

Merged
merged 25 commits into from
Aug 21, 2020
Merged

Conversation

zbaylin
Copy link
Member

@zbaylin zbaylin commented Aug 11, 2020

This should be a better version of #978. This uses Gtk's poll -> flush mechanism rather than using their event loop.

Unfortunately we can't make the Gtk window part of the window struct on Linux it seems, since the Window creation requires some initialization that isn't done by the point at which we create the window, leading to a segfault. In order to try to fix this I made a Hashtbl that maps Sdl windows to Gtk widgets so we don't end up with a bunch of duplicate Gtk widgets in memory.

@zbaylin zbaylin requested review from bryphe and glennsl August 11, 2020 13:43
Copy link
Member

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

I think this looks much better! There's one downside to this approach compared to using a single event queue though, that I neglected to mention earlier. The order of events between event systems aren't preserved here, so if some code assumes it is, it might fail unpredictably. This might not be a problem in practice, but it's hard to tell the future. I think it would be a good idea to document it at least.

src/Core/App.re Outdated Show resolved Hide resolved
src/Native/Gtk.re Outdated Show resolved Hide resolved
@zbaylin zbaylin requested a review from bryphe August 13, 2020 01:47
zbaylin and others added 3 commits August 13, 2020 08:31
Using long was only a safe assumption on certain architectures.
CI on Windows was breaking because of it.
@github-actions
Copy link

I have updated your lock dirs and formatted the code.
Please @zbaylin pull the last commit before pushing any more changes.

src/Native/Gtk.re Outdated Show resolved Hide resolved
@@ -22,7 +28,7 @@ open {
bunch of GtkWidgets for one window, so this table maps Sdl windows
to GtkWidgets.
*/
let windowToWidgetTable = WindowTbl.create(1);
let windowWidgetCache = WindowWidgetCache.create(~initialSize=8, 64);
Copy link
Member

Choose a reason for hiding this comment

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

What if you have more than 64 windows? Or, more likely, you create more than 64 temporary windows after the main window while continuing to use its associated widget?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I wonder if I can use the "depth" of the widget in the hierarchy as the weight in the cache. I'll look into that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the depth of the widget in the hierarchy to determine the weight of the item in the cache. This means root windows wont have to be recreated while temporary child windows will be.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Seems like a good strategy to me!

@bryphe
Copy link
Member

bryphe commented Aug 13, 2020

Change looks good to me (pending resolving Glenn's feedback).

Can you please test and verify Onivim 2 builds/runs with the latest changes?

@zbaylin
Copy link
Member Author

zbaylin commented Aug 14, 2020

@bryphe sure! Will do.

zbaylin and others added 4 commits August 14, 2020 17:07
This way child windows should be "lower" weighted than
the root widget/window.
@github-actions
Copy link

I have updated your lock dirs and formatted the code.
Please @zbaylin pull the last commit before pushing any more changes.

@zbaylin
Copy link
Member Author

zbaylin commented Aug 16, 2020

@bryphe sorry for the delay, I've been busy moving into my new apartment. I just tested with Oni and it works, I just had to resolve @opam/base to v0.14.0.

@zbaylin
Copy link
Member Author

zbaylin commented Aug 19, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zbaylin zbaylin requested a review from glennsl August 20, 2020 23:24
Copy link
Member

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to approve. LGTM now!

@zbaylin zbaylin merged commit f63a135 into master Aug 21, 2020
@Et7f3 Et7f3 deleted the feat/native/gtk-window branch May 6, 2021 13:33
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.

None yet

3 participants