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

set Display.errch and use it for errors from mouse/keyboard #17

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mjl-
Copy link

@mjl- mjl- commented Jan 6, 2018

i've only seen error EOF from devdraw mouse/keyboard, when i close
a window (at least on macos), or by calling Display.Close(). before
this change, the draw lib would call log.Fatal, ending the entire
program. now, it will send the error on errch. it is the caller's
responsibility to pass an errch to Init() that has buffering, if
it wants to be sure it sees the errors.

this doesn't remove the "TODO use errch" comment at Init(), because
not all errors are handled yet. there are still some log.Fatal's,
around allocimage/allocwindow.

i need this for applications that create multiple windows over their lifetime. before this change, closing any window log.Fatal's the entire program. i am launching multiple devdraw's. perhaps it's possible (or should) be to have devdraw manage multiple windows?

i've only seen error EOF from devdraw mouse/keyboard, when i close
a window (at least on macos), or by calling Display.Close().  before
this change, the draw lib would call log.Fatal, ending the entire
program.  now, it will send the error on errch.  it is the caller's
responsibility to pass an errch to Init() that has buffering, if
it wants to be sure it sees the errors.

this doesn't remove the "TODO use errch" comment at Init(), because
not all errors are handled yet. there are still some log.Fatal's,
around allocimage/allocwindow.
this now mounts $wsys to create a new window of the requested size.
it mounts the rio window at /n/duit.<window-name>, and opens its mouse/keyboard files, writes its label.

bugs: on interrupt (del) of the command, the window isn't removed.

this is loosely based on mischiefs draw9 library.
@rsc
Copy link
Contributor

rsc commented Apr 27, 2018

This seems wrong. If the caller passes a non-nil errch, great, then send errors there.
But if the caller has passed nil, it's not for Init to make an errch.
The caller is saying "I don't want to hear about errors."
In that case log.Fatal is fine.

@@ -0,0 +1,27 @@
// +build plan9

Choose a reason for hiding this comment

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

you can elide the build directive here - having _plan9.go file suffix is sufficient.

@@ -0,0 +1,129 @@
// +build plan9

Choose a reason for hiding this comment

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

same here - can elite build directive

@@ -0,0 +1,29 @@
// +build plan9

Choose a reason for hiding this comment

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

same here - can elite build directive

return nil, err
}
// note: must not close wsysfd, or fd's get mixed up...
d.mtpt = "/n/duit." + path.Base(wsys)

Choose a reason for hiding this comment

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

what's this? an artifact left over from testing/porting?

Choose a reason for hiding this comment

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

sorry, i didn't see the commit message before i wrote this.

why don't you follow plan9's newwindow and use /mnt/wsys with a bind over /dev?

Copy link
Author

Choose a reason for hiding this comment

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

i tried an rfork(RFNAMEG) briefly (like in newwindow), but it seemed to give me trouble with multiple go procs having different namespace. perhaps it's needed earlier in application setup, and only once, but i didn't want to go there (yet).
also i don't bind on (before) /dev because i want to make it possible for one application to have multiple windows (like in examples/alert).
i'm always using $wsys and a new window so writes to stdout/stderr don't clobber the ui. useful for me, at least in this stage. perhaps a first window should take over the window at /dev, and later windows should use $wsys...
i'm currently doing a mount on /n/ because mntgen is running there. if there's a way to create & access a window without a new mount, i'm interested, sounds easier. /mnt/wsys/wsys seems to be getting filled with new windows, but don't know what the contract is there.

btw, i've removed the build tags for plan9.

thanks for reviewing my changes.

Choose a reason for hiding this comment

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

yes, process specific state such as mount in go's multiprocess model on plan 9 can be a problem, similar to setuid/threads problem on unix.

i think the best thing to do would be give users the tools and documentation to solve this as they see fit. this would probably mean allowing a custom mount path to be passed, so you can specify that you want /mnt/wsys (with the caller doing rfork RFNAMEG) or /n/$id (without RFNAMEG).

in the former case, it would probably be good to document that if rfork RFNAMEG is used, runtime.LockOSThread must also be used, and operations on that window must happen inside that goroutine.

really, anyone intimate with plan9 and go would know about these problems. but it can't hurt to document it.

side note - there's no mntgen on plan 9 from bell labs. it's a 9front invention.

it isn't needed because of the "_plan9" suffix in the filename.
pointed out by nick owens.
@mjl-
Copy link
Author

mjl- commented Apr 29, 2018

about errch being assigned: i think i added a channel so other parts of the code could simply send (non-blocking) on it, without requiring "if errch != nil" checks in many places. can still be the wrong approach.

fwiw, i didn't intent to push the other changes (plan9-support) in this PR... i had forgotten that fork had a PR attached and github would add the new commits to the PR.
if there's interest in getting some of these changes merged, i can make cleaner changes.

knusbaum and others added 4 commits April 6, 2020 21:29
For one reason or another, the ctl file seems to be closed often when reattaching.
This patch preemptively reopens the ctl file when the window is attached or
reattached to make sure the handle is good.
draw: reopen ctl file when reattaching
draw: fix snarf for plan9 - correct snarf is always at /dev
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.

4 participants