Modal fixes (#281) #284
Modal fixes (#281) #284
Conversation
awfulcooking
force-pushed
the
modal-fixes
branch
from
bbaeb8d
to
26407d8
Compare
|
Looking good, thanks for this! Let me know when you think it's done. |
| @@ -156,12 +156,12 @@ func makeNewTab() *tab { | |||
| if t.hasContent() { | |||
| savePath, err := downloadPage(t.page) | |||
| if err != nil { | |||
| Error("Download Error", fmt.Sprintf("Error saving page content: %v", err)) | |||
| go Error("Download Error", fmt.Sprintf("Error saving page content: %v", err)) | |||
There was a problem hiding this comment.
My problem with this, and all the other go <modal> calls in this file is that it opens up the possibility of other things happening before the modal is dismissed. I'm not sure what the solution is though. Maybe all the code in t.view.SetInputCapture can be put in a goroutine (with a mutex that blocks multiple versions of it running) that blocks?
There was a problem hiding this comment.
This might be a good situation to ask about in #go-nuts on Libera. I wonder if they'd have any advice?
There was a problem hiding this comment.
Bear in mind the Info() and Error() functions previously just didn't block, but would return as soon as the modal had opened. Using go Info() should give similar behaviour to before, the only difference being that now the parent function may return before the modal has even rendered.
But if anything happened immediately after it the modals rendered, it would have shown UI breakage before #272, I think. And so we could expect this just to be a return to the previous, non-deadlocking behaviour
There was a problem hiding this comment.
Fun that we're addressing deadlocking and not-waiting-enough code in one PR :-)
As two sides of one coin.
There was a problem hiding this comment.
Bear in mind the Info() and Error() functions previously just didn't block, but would return as soon as the modal had opened. Using
go Info()should give similar behaviour to before, the only difference being that now the parent function may return before the modal has even rendered.
Yep, and that behavior worked well enough in general, so maybe it's fine. I'm just cautiious about future issues caused by this being non-blocking. But it's not a reason to not merge this PR or anything.
But if anything happened immediately after it the modals rendered, it would have shown UI breakage before #272
What makes you say this? Like it would draw focus away from the modal? And how does this PR change that? Seems like it could easily break things if something happened after still.
|
I think we should merge this, test it, and release after a few days if all seems well. Then improve from there! |
|
I'd rather test before merging, but in general this seems like a good fix for this issue, thanks for your work. As for a release, I gotta see, because I already have a new feature in Also please see my reply above. |
|
As for testing, it would probably be best to induce errors, like was done in #281, and make sure there are no UI issues. |
|
Going to merge this now, having done some testing myself. Thanks for making this patch. I don't intend to make a release for this, but it will be in v1.10.0 of course. If you have an idea about my comment above that would be appreciated. |
Pushing commits to this branch, feel free to test / merge, or wait for more to come in.