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

Rewrite GTK UI & Fix GTK UI Hang #62

Merged
merged 20 commits into from Mar 25, 2019

Conversation

Projects
None yet
5 participants
@mmstick
Copy link
Contributor

commented Aug 21, 2018

Rewrites the GTK UI to use an event-driven approach.

Closes #61

@brs17 brs17 requested a review from jackpot51 Sep 11, 2018

@brs17

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

@jackpot51 Unless you can show me how to recreate the deadlock, I would say you should go ahead and test the code and see if it fixes the issue you were having.

@jackpot51

This comment has been minimized.

Copy link
Member

commented Sep 12, 2018

I still get the hang on clicking on the next button. Here is what it says in the terminal:

$ pkexec popsicle-gtk ~/Downloads/pop-os_18.04_amd64_intel_debug_85.iso
switching to device selection
@jackpot51

This comment has been minimized.

Copy link
Member

commented Sep 12, 2018

It is likely caused by my USB SD card thingamajig. Maybe try one of those @brs17 ?

@brs17 brs17 self-requested a review Sep 27, 2018

@brs17
Copy link
Member

left a comment

This does not fix the issue. @mmstick I can show you in the lab what's going on if you'd like.

@mmstick

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

May not be a deadlock. About to add some debug print statements that might give the answer.

@mmstick mmstick force-pushed the hang branch from 2cd8a4c to 6db2927 Sep 28, 2018

@mmstick mmstick changed the title Attempt to fix possible deadlock Fix GTK UI hang when checking block device sizes Sep 28, 2018

@mmstick

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Edit: Yes, this is the issue.

I suspect it's due to a card reader returning 0 as the number of sectors, and the loop condition that scans for that value not being 0 is in the incorrect order. It should check the number of attempts first, then the value.

@mmstick mmstick force-pushed the hang branch from 6db2927 to 70e4232 Sep 28, 2018

@mmstick

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

This seems to have fixed it.

@mmstick mmstick force-pushed the hang branch from c04dc44 to 5955ead Sep 28, 2018

@mmstick mmstick force-pushed the hang branch from 5955ead to 85f0cdf Sep 28, 2018

@brs17 brs17 self-requested a review Oct 8, 2018

@brs17
Copy link
Member

left a comment

Hang still occurs

@mmstick

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

😢 I must have broken it with 85f0cdf

@brs17 brs17 self-requested a review Nov 19, 2018

@brs17
Copy link
Member

left a comment

Popsicle still hangs.

@mmstick

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

I'll look into it when I have more time.

@brs17

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

Definitely not a priority, but now we at least know the issue isn't fixed yet.

@mmstick

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Spent the weekend rewriting all of the logic in the GTK frontend. It now uses an event system similar to that in use by the receiving UI. There's much less moving pieces as a result, and this may have fixed the UI hanging.

@mmstick mmstick changed the title Fix GTK UI hang when checking block device sizes Rewrite GTK UI & Fix GTK UI Hang Feb 11, 2019

@brs17 brs17 self-requested a review Feb 11, 2019

@mmstick mmstick force-pushed the hang branch from 8d2a184 to fa14b7d Feb 11, 2019

@ahoneybun

This comment has been minimized.

Copy link

commented Feb 11, 2019

Latest push fixed the issue for me and going to see if the new live disk boots.

Edit: The live disk boots and everything seems to work. LGTM.

@mmstick

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

I thought it might fix it. The previous design of the event system had potential for getting stuck in a lock, due to the large amounts of threads locking and unlocking the same locks in the background. Dynamically checking for the need to refresh devices on the device list caused a lot of overhead in additional bookkeeping.

The new design simply sends all UI events through channels to a single background thread, which later responds through a channel to a single GTK event loop in the main thread. That way, clicking a button doesn't spawn a new thread, but queues a new event to be handled in the background on an existing thread.

Seems those working on the GLib bindings for Rust have come to a similar conclusion, that a channel-based approach is more ideal for handling UI events. A new version of glib-rs will have it's own channel mechanism that can register with the glib main event loop.

let mut tasks = None;

gtk::timeout_add(16, move || {
match state.ui_event_rx.try_recv() {

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 28, 2019

Instead of polling the channel, you probably want to properly integrate with the GLib main loop/context. You already use the crossbeam channel so probably care about performance, and this here kind of defeats the purpose.

Look how it's done in glib-rs for the integrated channel.

This comment has been minimized.

Copy link
@mmstick

mmstick Feb 28, 2019

Author Contributor

Is that in a stable release yet?

This comment has been minimized.

Copy link
@sdroege

@mmstick mmstick merged commit 8e2c0b0 into master Mar 25, 2019

6 checks passed

pop-os/staging/bionic/binary Pop!_OS Staging bionic/binary
Details
pop-os/staging/bionic/source Pop!_OS Staging bionic/source
Details
pop-os/staging/cosmic/binary Pop!_OS Staging cosmic/binary
Details
pop-os/staging/cosmic/source Pop!_OS Staging cosmic/source
Details
pop-os/staging/disco/binary Pop!_OS Staging disco/binary
Details
pop-os/staging/disco/source Pop!_OS Staging disco/source
Details

@jackpot51 jackpot51 deleted the hang branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.