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

Script to spawn a browser / start notebook. #693

Closed
boothby opened this issue Sep 19, 2007 · 31 comments
Closed

Script to spawn a browser / start notebook. #693

boothby opened this issue Sep 19, 2007 · 31 comments

Comments

@boothby
Copy link

boothby commented Sep 19, 2007

I've had an icon sitting on my desktop for about a week now. When I click on it, and it starts a notebook in a background terminal and spawns a browser. I'd like to be able to click it a second time, and open another browser window, instead of the current behavior of attempting to start another notebook.

Should work something like this:

if not notebook_is_running:
    notebook(settings from commandline, open_browser=True)
else:
    open_browser(settings from commandline)

Apply attachment: trac_693-spawn_notebook.3.patch

CC: @williamstein @qed777 @wjp @sagetrac-acleone @mwhansen @jdemeyer @sagetrac-mvngu

Component: notebook

Author: Tim Dumol, Mitesh Patel

Reviewer: Ivan Andrus

Merged: sage-4.7.alpha3

Issue created by migration from https://trac.sagemath.org/ticket/693

@boothby boothby added this to the sage-4.7 milestone Sep 19, 2007
@boothby boothby self-assigned this Sep 19, 2007
@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 15, 2009

Modifies sage -notebook to launch a browser window if the notebook is already started. Also adds a sage -nb shortcut.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 15, 2009

comment:1

Attachment: trac_693-spawn-browser-start-nb.patch.gz

This patch should do the trick. Apply to scripts repository.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 15, 2009

Author: Tim Dumol

@TimDumol TimDumol mannequin added the s: needs review label Nov 15, 2009
@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 18, 2009

Attachment: trac_693-spawn-browser-start-nb.2.patch.gz

Checks if the notebook server is running too, instead of just checking if the PID exists.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 18, 2009

comment:2

I am not sure which method is preferrable, since checking if the notebook server is running does not work if the user has no permission to read /proc and to send signals. Please feel free to approve of either patch.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 19, 2009

Attachment: trac_693-spawn-browser-start-nb.3.patch.gz

Fixes bug with arguments.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 19, 2009

Attachment: trac_693-spawn-browser-start-nb.4.patch.gz

Same thing, without actually checking if the Twistd process is running

@williamstein
Copy link
Contributor

comment:3

I don't get it. If I do

$  sage -notebook directory=foo port=8001 & 
$  sage -notebook directory=bar port=8002 

then when I execute the second line it might just pop up a notebook server pointed at port 8001. Actually, given the line:

cmd = "notebook(" + ",".join([wrap(v) for v in sys.argv[1:]]) + ",port=" + SAGENB_PORT + ")"

I think it would give an error, since port= is specified twice.

This is because you introduced a new environment variable SAGENB_PORT which isn't documented. I don't know why it is there. I think you should get the port from the port= option on the command line.

I think you should get port= from the command line and get rid of the SAGENB_PORT environment variable.

Also, you use:

DOT_SAGENB = os.environ.get('DOT_SAGENB', os.path.join(os.environ['HOME'], '.sage', 'sage_notebook.sagenb'))

but actually, you need to use the file os.path.join(D, 'twistd.pid') where D is the option specified in directory= in the command line.

Finally, I think this code should be in the notebook(...) command in Sage itself. It's wrong putting it here in this shell script, because it only half way fixes the problem. E.g., imagine a user that types the following and leaves that in a console:

sage: notebook()

Then in another console, they type

sage: notebook()

Instead of giving an error, it should just given them the notebook.
If you put this code that you've just written in the notebook command, then both problems are automatically solved, whereas with the current code location, only half the problem is really solved.

Also, there is a notebook(fork=True) option, so one can do

sage: notebook(fork=True)
The notebook files are stored in: sage_notebook.sagenb
**************************************************
*                                                *
* Open your web browser to http://localhost:8001 *
*                                                *
**************************************************
<pexpect.spawn instance at 0x10bb78bd8>
sage: notebook()
# get some notebook

William

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 21, 2009

Attachment: trac_693-spawn-nb.patch.gz

Changes run_notebook.py to launch a browser to the notebook page should an instance in the directory exist. Apply to sagenb repo. Apply this patch only.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Nov 21, 2009

comment:4

All your points make sense. I have implemented the changes in run_notebook.py. I've decided to check if the process is running, since that's also what twistd does.

@TimDumol TimDumol mannequin added s: needs review and removed s: needs work labels Nov 21, 2009
@williamstein
Copy link
Contributor

comment:5

On OS X this doesn't work at all. Depending on what I do either I get two notebook servers running simultaneously on the same directory (bad), or I get "Another twistd server is running, PID 40940".

On OS X there is no /proc filesystem. However, when I run this code from this patch:

        try:
            print 1
            # First check using /proc directory
            if os.path.exists('/proc/%d'  % twistd_pid):
                launch_browser_to_nb()
                return
            else:
                remove_pidfile(twistd_pid_path)                
        except:
            print 2

I don't see "2" printed, i.e., no exception is raised. That's clear if you read the code -- you don't raise an exception.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Dec 9, 2009

Attachment: trac_693-spawn-nb.2.patch.gz

Uses signals only to check if the process exists (as Twisted does)

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Dec 9, 2009

comment:6

I have removed the /proc check, since it's what Twisted does anyways.

@TimDumol TimDumol mannequin added s: needs review and removed s: needs work labels Dec 9, 2009
@qed777
Copy link
Mannequin

qed777 mannequin commented Jan 20, 2010

comment:7

I guess we need to update the patch to take advantage of #2779?

@qed777
Copy link
Mannequin

qed777 mannequin commented Jan 25, 2010

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

@qed777
Copy link
Mannequin

qed777 mannequin commented Jan 25, 2010

comment:8

Attachment: trac_693-spawn_notebook.3.patch.gz

V3:

  • If a server is running and open_viewer=True, get the settings from the old twistedconf.tac and browse to the server's home page.
  • Use return instead of sys.exit, in case of command-line invocation.
  • Some pep8 changes.
  • Rebased for this queue
sagenb-0.7 / #8051
trac_7784-hgignore_update.2.patch
trac_5712-interrupt-notification.6.patch
trac_6069-missing_pub_ws.2.patch
trac_8038-email_plus_addressing_v2.patch
trac_7506-notebook_object-documentation.2.patch
trac_693-spawn_notebook.3.patch

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Mar 19, 2010

Reviewer: Tim Dumol

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Mar 19, 2010

Changed author from Tim Dumol to Tim Dumol, Mitesh Patel

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Mar 19, 2010

Changed reviewer from Tim Dumol to none

@qed777
Copy link
Mannequin

qed777 mannequin commented Jun 10, 2010

comment:11
applying trac_693-spawn_notebook.3.patch
patching file sagenb/notebook/run_notebook.py
Hunk #12 succeeded at 434 with fuzz 2 (offset 3 lines).
now at: trac_693-spawn_notebook.3.patch

@gvol
Copy link

gvol commented Dec 29, 2010

comment:13

Apply trac_693-spawn_notebook.3.patch

@kcrisman
Copy link
Member

comment:14

I'd like to test #8473, which depends on this, but I'm reluctant to do so until someone who knows something about the notebook takes a look at this. Bug days folks?

@ghost
Copy link

ghost commented Mar 15, 2011

comment:15

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

air jordan

@kcrisman
Copy link
Member

comment:16

Replying to amog2011:

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

This is a spam comment. Can someone please remove this 'user'? I don't know who has permissions for this.

@jdemeyer
Copy link

comment:17

Replying to @kcrisman:

Replying to amog2011:

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

This is a spam comment. Can someone please remove this 'user'? I don't know who has permissions for this.

Minh has.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Mar 16, 2011

comment:18

Replying to @kcrisman:

This is a spam comment. Can someone please remove this 'user'? I don't know who has permissions for this.

amog2011 is already banned.

@gvol
Copy link

gvol commented Mar 24, 2011

comment:19

I have been using this (with #8473) for some time without problems. I have also reviewed the code and it looks okay given my limited understanding of the notebook.

@kcrisman
Copy link
Member

comment:20

Replying to @gvol:

I have been using this (with #8473) for some time without problems. I have also reviewed the code and it looks okay given my limited understanding of the notebook.

Thanks. It applies just as cleanly (one hunk misses with fuzz) to current SageNB as in the comment above. This could be added in a new spkg with the Jmol updates.

Apply attachment: trac_693-spawn_notebook.3.patch

@kcrisman
Copy link
Member

Reviewer: Ivan Andrus

@kcrisman

This comment has been minimized.

@jdemeyer
Copy link

Merged: sage-4.7.alpha3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants