[9front] delete in page(1)

[9front] delete in page(1)

[qwx]

Hi,

I wrote a patch allowing one to remove(2) the current file
directly from page(1) with the `d' key.  I've needed this
for instance for pruning image albums, especially with
shuffled lists, etc.  I've tried other strategies instead
of adding this, but they all sucked.

I don't know if anyone wants this merged since it doesn't
seem like something people do much, but in any case here
it is.  Thoughts and comments appreciated as always.

Prost,
qwx


diff 855cf4326f5a07d7142c2d8918f5fa856d912b85 uncommitted
--- a//sys/src/cmd/page.c
+++ b//sys/src/cmd/page.c
@@ -74,6 +74,7 @@
 	Czerox,
 	Cwrite,
 	Cext,
+	Cdel,
 	Cdummy2,
 	Cquit,
 };
@@ -98,6 +99,7 @@
 	[Czerox]	"zerox",	'z', 0, 0,
 	[Cwrite]	"write",	'w', 0, 0,
 	[Cext]		"ext",		'x', 0, 0,
+	[Cdel]		"del",		'd', 0, 0,
 	[Cdummy2]	"",		0, 0, 0,
 	[Cquit]		"quit",		'q', Kdel, Keof,
 };
@@ -134,6 +136,7 @@
 void showpage(Page *);
 void drawpage(Page *);
 Point pagesize(Page *);
+void drawlock(int);
 
 Page*
 addpage(Page *up, char *name, int (*popen)(Page *), void *pdata, int fd)
@@ -986,6 +989,54 @@
 	}
 }
 
+/* page entries are never freed, there's no point
+ * and would break everything */
+Page*
+delpage(Page *p)
+{
+	Page *t, *q;
+
+	if(p == nil)
+		return nil;
+	/* to remove(2) subpages in documents makes no sense, and just
+	 * removing a subentry doesn't seem like a feature worth the bother */
+	if(p->up != root)
+		return p;
+	if(p->fd >= 0)
+		close(p->fd);
+	p->fd = -1;
+	if(remove(p->name) < 0){
+		fprint(2, "remove %s: %r", p->name);
+		return p;
+	}
+	qlock(&pagelock);
+	for(t = p->down; t != nil && t->up != root; t = q){
+		qlock(t);
+		drawlock(0);
+		unloadpage(t);
+		drawlock(1);
+		free(t->name);
+		free(t->data);
+		t->name = t->data = nil;
+		q = nextpage(t);
+		qunlock(t);
+	}
+	drawlock(0);
+	unloadpage(p);
+	drawlock(1);
+	free(p->name);
+	free(p->data);
+	p->name = p->data = nil;
+	if(root->down != p){
+		t = prevpage(p);
+		t->next = p->next;
+	}else
+		root->down = p->next;
+	qunlock(&pagelock);
+	qunlock(p);
+	return p->next != nil ? p->next : t;
+}
+
 /*
  * A draw operation that touches only the area contained in bot but not in top.
  * mp and sp get aligned with bot.min.
@@ -1461,6 +1512,7 @@
 	char buf[NPATH], *s;
 	Point o;
 	int fd;
+	Page *p;
 
 	switch(i){
 	case Corigsize:
@@ -1545,6 +1597,17 @@
 		break;
 	case Csnarf:
 		writeaddr(current, "/dev/snarf");
+		break;
+	case Cdel:
+		if(current == nil || !canqlock(current))
+			break;
+		if((p = delpage(current)) == current)
+			break;
+		current = p;
+		if(current == nil)
+			break;
+		forward = 1;
+		showpage(current);
 		break;
 	case Cnext:
 		forward = 1;

Re: [9front] delete in page(1)

[umbraticus]

Well, that was a good lesson in reading first!
Dump to the rescue...
So the feature I assumed this added was just
removing images from the list, which could
be handy since I usually have a persistent
page catching everything plumbed its way.
I may help myself to just that part of the patch...

Anyway, can confirm it does what it says :)
Removing the last item in the list did seem
to break the b3 menu tho...

umbraticus

Re: [9front] delete in page(1)

[qwx]

On Fri Dec 31 02:59:46 +0200 2021, umbraticus@prosimetrum.com wrote:
> Well, that was a good lesson in reading first!
> Dump to the rescue...
> So the feature I assumed this added was just
> removing images from the list, which could
> be handy since I usually have a persistent
> page catching everything plumbed its way.
> I may help myself to just that part of the patch...

Ouch, might not have explained it clearly enough!  That's
easy to do, you could have both since the only part that
changes is basically the remove(2) call.


> Anyway, can confirm it does what it says :)
> Removing the last item in the list did seem
> to break the b3 menu tho...

Ah, that works fine for me.  I'll have to test it some
more then.  Thanks!

qwx

Re: [9front] delete in page(1)

[qwx]

diff 855cf4326f5a07d7142c2d8918f5fa856d912b85 uncommitted
--- a//sys/src/cmd/page.c
+++ b//sys/src/cmd/page.c
@@ -74,6 +74,7 @@
 	Czerox,
 	Cwrite,
 	Cext,
+	Cdel,
 	Cdummy2,
 	Cquit,
 };
@@ -98,6 +99,7 @@
 	[Czerox]	"zerox",	'z', 0, 0,
 	[Cwrite]	"write",	'w', 0, 0,
 	[Cext]		"ext",		'x', 0, 0,
+	[Cdel]		"del",		'd', 0, 0,
 	[Cdummy2]	"",		0, 0, 0,
 	[Cquit]		"quit",		'q', Kdel, Keof,
 };
@@ -134,6 +136,7 @@
 void showpage(Page *);
 void drawpage(Page *);
 Point pagesize(Page *);
+void drawlock(int);
 
 Page*
 addpage(Page *up, char *name, int (*popen)(Page *), void *pdata, int fd)
@@ -986,6 +989,54 @@
 	}
 }
 
+/* page entries are never freed, there's no point
+ * and would break everything */
+Page*
+delpage(Page *p)
+{
+	Page *t, *q;
+
+	if(p == nil)
+		return nil;
+	/* to remove(2) subpages in documents makes no sense, and just
+	 * removing a subentry doesn't seem like a feature worth the bother */
+	if(p->up != root)
+		return p;
+	if(p->fd >= 0)
+		close(p->fd);
+	p->fd = -1;
+	if(remove(p->name) < 0){
+		fprint(2, "remove %s: %r", p->name);
+		return p;
+	}
+	qlock(&pagelock);
+	for(t = p->down; t != nil && t->up != root; t = q){
+		qlock(t);
+		drawlock(0);
+		unloadpage(t);
+		drawlock(1);
+		free(t->name);
+		free(t->data);
+		t->name = t->data = nil;
+		q = nextpage(t);
+		qunlock(t);
+	}
+	drawlock(0);
+	unloadpage(p);
+	drawlock(1);
+	free(p->name);
+	free(p->data);
+	p->name = p->data = nil;
+	if(root->down != p){
+		t = prevpage(p);
+		t->next = p->next;
+	}else
+		root->down = p->next;
+	qunlock(&pagelock);
+	qunlock(p);
+	return p->next != nil ? p->next : t;
+}
+
 /*
  * A draw operation that touches only the area contained in bot but not in top.
  * mp and sp get aligned with bot.min.
@@ -1461,6 +1512,7 @@
 	char buf[NPATH], *s;
 	Point o;
 	int fd;
+	Page *p;
 
 	switch(i){
 	case Corigsize:
@@ -1545,6 +1597,19 @@
 		break;
 	case Csnarf:
 		writeaddr(current, "/dev/snarf");
+		break;
+	case Cdel:
+		if(current == nil || !canqlock(current))
+			break;
+		if((p = delpage(current)) == current){
+			qunlock(current);
+			break;
+		}
+		current = p;
+		if(current == nil)
+			break;
+		forward = 1;
+		showpage(current);
 		break;
 	case Cnext:
 		forward = 1;

Re: [9front] delete in page(1)

[umbraticus]

hmm, still messes up. what I am doing:
1. plumb 1.jpg, opens in new page
2. press d
3. plumb 2.jpg → shows image

can continue to plumb new images but b3menu doesn't open
and prev/next doesn't work.

Also, maybe at least have the key be D instead of d to prevent mistakes...

umbraticus

Re: [9front] delete in page(1)

[qwx]

On Fri Dec 31 04:16:49 +0200 2021, umbraticus@prosimetrum.com wrote:
> hmm, still messes up. what I am doing:
> 1. plumb 1.jpg, opens in new page
> 2. press d
> 3. plumb 2.jpg → shows image
> 
> can continue to plumb new images but b3menu doesn't open
> and prev/next doesn't work.
> 
> Also, maybe at least have the key be D instead of d to prevent mistakes...
> 
> umbraticus

Here's a patched patch fixing this and another issue;  it
also has the delete command on the `D' key as suggested and
has a separate `pop' command on the `p' key to only remove
an entry from the list, without deleting the file.  Thoughts?

Thanks,
qwx


diff 855cf4326f5a07d7142c2d8918f5fa856d912b85 uncommitted
--- a/sys/src/cmd/page.c
+++ b/sys/src/cmd/page.c
@@ -74,7 +74,10 @@
 	Czerox,
 	Cwrite,
 	Cext,
+	Cpop,
 	Cdummy2,
+	Cdelete,
+	Cdummy3,
 	Cquit,
 };
 
@@ -98,7 +101,10 @@
 	[Czerox]	"zerox",	'z', 0, 0,
 	[Cwrite]	"write",	'w', 0, 0,
 	[Cext]		"ext",		'x', 0, 0,
+	[Cpop]		"pop",		'p', 0, 0,
 	[Cdummy2]	"",		0, 0, 0,
+	[Cdelete]	"delete",	'D', 0, 0,
+	[Cdummy3]	"",		0, 0, 0,
 	[Cquit]		"quit",		'q', Kdel, Keof,
 };
 
@@ -134,6 +140,7 @@
 void showpage(Page *);
 void drawpage(Page *);
 Point pagesize(Page *);
+void drawlock(int);
 
 Page*
 addpage(Page *up, char *name, int (*popen)(Page *), void *pdata, int fd)
@@ -986,6 +993,71 @@
 	}
 }
 
+/* page entries are never freed, there's no point
+ * and would break everything */
+Page*
+poppage(Page *p)
+{
+	Page *t, *q;
+
+	if(p == nil)
+		return nil;
+	if(p->up != root)
+		return p;
+	qlock(&pagelock);
+	for(t = p->down; t != nil && t->up != root; t = q){
+		qlock(t);
+		drawlock(0);
+		unloadpage(t);
+		drawlock(1);
+		free(t->name);
+		free(t->data);
+		t->name = t->data = nil;
+		q = nextpage(t);
+		qunlock(t);
+	}
+	drawlock(0);
+	unloadpage(p);
+	drawlock(1);
+	free(p->name);
+	free(p->data);
+	p->name = p->data = nil;
+	t = prevpage(p);
+	if(root->tail == p)
+		root->tail = t;
+	if(root->down == p || t == nil)
+		root->down = p->next;
+	else
+		t->next = p->next;
+	qunlock(&pagelock);
+	qunlock(p);
+	if(p->next != nil){
+		forward = 1;
+		return p->next;
+	}
+	forward = -1;
+	return t;
+}
+
+Page*
+delpage(Page *p)
+{
+	if(p == nil)
+		return nil;
+	/* to remove(2) subpages in documents makes no sense, and just
+	 * removing a subentry doesn't seem like a feature worth the bother */
+	if(p->up != root)
+		return p;
+	if(p->fd >= 0)
+		close(p->fd);
+	p->fd = -1;
+	if(remove(p->name) < 0){
+		fprint(2, "remove %s: %r", p->name);
+		return p;
+	}
+	return poppage(p);
+}
+
 /*
  * A draw operation that touches only the area contained in bot but not in top.
  * mp and sp get aligned with bot.min.
@@ -1461,6 +1533,7 @@
 	char buf[NPATH], *s;
 	Point o;
 	int fd;
+	Page *p;
 
 	switch(i){
 	case Corigsize:
@@ -1546,6 +1619,28 @@
 	case Csnarf:
 		writeaddr(current, "/dev/snarf");
 		break;
+	case Cpop:
+		if(current == nil || !canqlock(current))
+			break;
+		if((p = poppage(current)) == current)
+			break;
+	Reset:
+		current = p;
+		if(current == nil){
+			drawlock(0);
+			draw(screen, screen->r, paper, nil, ZP);
+			drawframe(screen->r);
+			drawlock(1);
+			break;
+		}
+		showpage(current);
+		break;
+	case Cdelete:
+		if(current == nil || !canqlock(current))
+			break;
+		if((p = delpage(current)) == current)
+			break;
+		goto Reset;
 	case Cnext:
 		forward = 1;
 		showpage(nextpage(current));

Re: [9front] delete in page(1)

[umbraticus]

Nice work, thanks! I like the isolation in the menu too.

I found another nit :)

Popping a doc eg. /sys/doc/troff.ps works fine
if you are at the root window of the document
(the grey one with name of the file in a box)
and popping from within the doc doesn't work,
which is fine if that's how you want it to behave
but if I open the doc, try to pop from eg. page 3,
then go back to the root and try to pop from
there things hang...

umbraticus

Re: [9front] delete in page(1)

[qwx]

On Sat Jan  1 12:50:01 +0200 2022, umbraticus@prosimetrum.com wrote:
> Nice work, thanks! I like the isolation in the menu too.
> 
> I found another nit :)
> 
> Popping a doc eg. /sys/doc/troff.ps works fine
> if you are at the root window of the document
> (the grey one with name of the file in a box)
> and popping from within the doc doesn't work,
> which is fine if that's how you want it to behave
> but if I open the doc, try to pop from eg. page 3,
> then go back to the root and try to pop from
> there things hang...
> 
> umbraticus

Thanks for testing!  Just another locking fuckup,
a bit of a nightmare.

qwx


diff c4e30bede2f79c42dacb8c91713abf4a4d91c45c uncommitted
--- a//sys/src/cmd/page.c
+++ b//sys/src/cmd/page.c
@@ -74,7 +74,10 @@
 	Czerox,
 	Cwrite,
 	Cext,
+	Cpop,
 	Cdummy2,
+	Cdelete,
+	Cdummy3,
 	Cquit,
 };
 
@@ -98,7 +101,10 @@
 	[Czerox]	"zerox",	'z', 0, 0,
 	[Cwrite]	"write",	'w', 0, 0,
 	[Cext]		"ext",		'x', 0, 0,
+	[Cpop]		"pop",		'p', 0, 0,
 	[Cdummy2]	"",		0, 0, 0,
+	[Cdelete]	"delete",	'D', 0, 0,
+	[Cdummy3]	"",		0, 0, 0,
 	[Cquit]		"quit",		'q', Kdel, Keof,
 };
 
@@ -134,6 +140,7 @@
 void showpage(Page *);
 void drawpage(Page *);
 Point pagesize(Page *);
+void drawlock(int);
 
 Page*
 addpage(Page *up, char *name, int (*popen)(Page *), void *pdata, int fd)
@@ -986,6 +993,71 @@
 	}
 }
 
+/* page entries are never freed, there's no point
+ * and would break everything */
+Page*
+poppage(Page *p)
+{
+	Page *t, *q;
+
+	if(p == nil)
+		return nil;
+	if(p->up != root)
+		return p;
+	qlock(&pagelock);
+	for(t = p->down; t != nil && t->up != root; t = q){
+		qlock(t);
+		drawlock(0);
+		unloadpage(t);
+		drawlock(1);
+		free(t->name);
+		free(t->data);
+		t->name = t->data = nil;
+		q = nextpage(t);
+		qunlock(t);
+	}
+	drawlock(0);
+	unloadpage(p);
+	drawlock(1);
+	free(p->name);
+	free(p->data);
+	p->name = p->data = nil;
+	t = prevpage(p);
+	if(root->tail == p)
+		root->tail = t;
+	if(root->down == p || t == nil)
+		root->down = p->next;
+	else
+		t->next = p->next;
+	qunlock(&pagelock);
+	qunlock(p);
+	if(p->next != nil){
+		forward = 1;
+		return p->next;
+	}
+	forward = -1;
+	return t;
+}
+
+Page*
+delpage(Page *p)
+{
+	if(p == nil)
+		return nil;
+	/* to remove(2) subpages in documents makes no sense, and just
+	 * removing a subentry doesn't seem like a feature worth the bother */
+	if(p->up != root)
+		return p;
+	if(p->fd >= 0)
+		close(p->fd);
+	p->fd = -1;
+	if(remove(p->name) < 0){
+		fprint(2, "remove %s: %r", p->name);
+		return p;
+	}
+	return poppage(p);
+}
+
 /*
  * A draw operation that touches only the area contained in bot but not in top.
  * mp and sp get aligned with bot.min.
@@ -1461,6 +1533,7 @@
 	char buf[NPATH], *s;
 	Point o;
 	int fd;
+	Page *p;
 
 	switch(i){
 	case Corigsize:
@@ -1546,6 +1619,32 @@
 	case Csnarf:
 		writeaddr(current, "/dev/snarf");
 		break;
+	case Cpop:
+		if(current == nil || !canqlock(current))
+			break;
+		if((p = poppage(current)) == current){
+			qunlock(current);
+			break;
+		}
+	Reset:
+		current = p;
+		if(current == nil){
+			drawlock(0);
+			draw(screen, screen->r, paper, nil, ZP);
+			drawframe(screen->r);
+			drawlock(1);
+			break;
+		}
+		showpage(current);
+		break;
+	case Cdelete:
+		if(current == nil || !canqlock(current))
+			break;
+		if((p = delpage(current)) == current){
+			qunlock(current);
+			break;
+		}
+		goto Reset;
 	case Cnext:
 		forward = 1;
 		showpage(nextpage(current));

Re: [9front] delete in page(1)

[umbraticus]

works great now; pretty sure I'm done kicking the tyres.
thanks!

umbraticus

Re: [9front] delete in page(1)

[qwx]

> I like this new feature too but am able to crash by popping pages of
> a large pdf. At first I thought it only happened when the whole doc
> had not yet loaded but I think it may also occur even when every
> thing is in memory. Of course being able to drop pages from a pdf is
> probably of questionable utility...

In fact it occurs all the time in pdfs, probably elsewhere.  It's a
stupid bug that should be fixed with this new patch.  The patch
actually doesn't distinguish between pages of a pdf or files in a
subdirectory.  Previously, you couldn't pop or delete anything that
wasn't directly at the root, meaning that if you did `page $dir',
you would not be able to do anything to pages under $dir.  Hopefully
this thing is done and dusted for now...

Thanks,
qwx


diff 9d43029ff984435111eff658308a44b4f3eee1cc uncommitted
--- a//sys/src/cmd/page.c
+++ b//sys/src/cmd/page.c
@@ -74,7 +74,10 @@
 	Czerox,
 	Cwrite,
 	Cext,
+	Cpop,
 	Cdummy2,
+	Cdelete,
+	Cdummy3,
 	Cquit,
 };
 
@@ -98,7 +101,10 @@
 	[Czerox]	"zerox",	'z', 0, 0,
 	[Cwrite]	"write",	'w', 0, 0,
 	[Cext]		"ext",		'x', 0, 0,
+	[Cpop]		"pop",		'p', 0, 0,
 	[Cdummy2]	"",		0, 0, 0,
+	[Cdelete]	"delete",	'D', 0, 0,
+	[Cdummy3]	"",		0, 0, 0,
 	[Cquit]		"quit",		'q', Kdel, Keof,
 };
 
@@ -134,6 +140,7 @@
 void showpage(Page *);
 void drawpage(Page *);
 Point pagesize(Page *);
+void drawlock(int);
 
 Page*
 addpage(Page *up, char *name, int (*popen)(Page *), void *pdata, int fd)
@@ -987,6 +994,72 @@
 	}
 }
 
+/* doesn't actually free the page entry or touch links to avoid breakage */
+Page*
+freepage(Page *p, Page *prev)
+{
+	Page *next, *up;
+
+	drawlock(0);
+	unloadpage(p);
+	drawlock(1);
+	if(p->fd >= 0)
+		close(p->fd);
+	p->fd = -1;
+	/* not touching p->data */
+	free(p->name);
+	p->name = nil;
+	p->open = nil;
+	next = nextpage(p);
+	up = p->up;
+	if(up->down == p){
+		if(up->tail != p)
+			up->down = next;
+		else
+			up->down = nil;
+	}else if(up->tail == p){
+		up->tail = prev;
+		prev->next = nil;
+	}else
+		prev->next = next;
+	return next;
+}
+
+Page*
+poppage(Page *p, int del)
+{
+	Page *t, *prev, *next;
+
+	if(p == nil)
+		return nil;
+	if(p == root)
+		return p;
+	if(del){
+		if(!(access(p->name, OREAD) == 0 && remove(p->name) == 0
+		|| p->data != nil && access(p->data, OREAD) == 0 && remove(p->data) == 0)){
+			fprint(2, "remove %s: %r", p->name);
+			return p;
+		}
+	}
+	qlock(&pagelock);
+	for(t = p->down, prev = p; t != nil && t->up != p->up; prev = t, t = next){
+		qlock(t);
+		next = freepage(t, prev);
+		qunlock(t);
+	}
+	p->down = nil;
+	prev = prevpage(p);
+	next = freepage(p, prev);
+	qunlock(&pagelock);
+	qunlock(p);
+	if(next != nil){
+		forward = 1;
+		return next;
+	}
+	forward = -1;
+	return prev;
+}
+
 /*
  * A draw operation that touches only the area contained in bot but not in top.
  * mp and sp get aligned with bot.min.
@@ -1462,8 +1535,10 @@
 {
 	char buf[NPATH], *s;
 	Point o;
-	int fd;
+	int fd, del;
+	Page *p;
 
+	del = 0;
 	switch(i){
 	case Corigsize:
 		pos = ZP;
@@ -1547,6 +1622,25 @@
 		break;
 	case Csnarf:
 		writeaddr(current, "/dev/snarf");
+		break;
+	case Cdelete:
+		del = 1;
+		/* wet floor */
+	case Cpop:
+		if(current == nil || !canqlock(current))
+			break;
+		if((p = poppage(current, del)) == current){
+			qunlock(current);
+			break;
+		}
+		if((current = p) == nil){
+			drawlock(0);
+			draw(screen, screen->r, paper, nil, ZP);
+			drawframe(screen->r);
+			drawlock(1);
+			break;
+		}
+		showpage(current);
 		break;
 	case Cnext:
 		forward = 1;

Re: [9front] delete in page(1)

[umbraticus]

Thanks for the performance tweaks.  I had bumped imemlimit, which
makes moving around in big pdfs nicer but not until the whole thing
is loaded. With the increased responsiveness I may reduce it again.

I like this new feature too but am able to crash by popping pages of
a large pdf. At first I thought it only happened when the whole doc
had not yet loaded but I think it may also occur even when every
thing is in memory. Of course being able to drop pages from a pdf is
probably of questionable utility...

; lstk
/proc/7624/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: abort()+0x0 /sys/src/libc/9sys/abort.c:6
ppanic(fmt=0x406df9)+0x14b /sys/src/libc/port/malloc.c:166
	pv=0x4087d8
	msg=0x409010
	v=0x7fffffffc3f8
	n=0xffffc3f80000002d
D2B(v=0x41b078)+0x6a /sys/src/libc/port/pool.c:926
	a=0x41b070
poolfreel(v=0x41b078,p=0x4065d0)+0x23 /sys/src/libc/port/pool.c:1153
	ab=0x4087d8
poolfree(p=0x4065d0,v=0x41b078)+0x3e /sys/src/libc/port/pool.c:1287
free()+0x26 /sys/src/libc/port/malloc.c:250
freepage(p=0x4208c0,prev=0x4207e0)+0x5b /usr/umbraticus/src/local/page.c:1010
poppage(p=0x4208c0,del=0xe00000000)+0xb4 /usr/umbraticus/src/local/page.c:1051
	prev=0x4207e0
	t=0x41b920
	next=0x20a898
docmd(m=0x7fffffffead0,i=0xe)+0x622 /usr/umbraticus/src/local/page.c:1631
	del=0x0
	o=0x214dbf
	buf=0x4569c0
	fd=0x214dbf00000000
main(argc=0x0,argv=0x7fffffffefa8)+0xa11 /usr/umbraticus/src/local/page.c:1794
	_argc=0x69
	_args=0x0
	m=0x900000000
	i=0x41fd2000000001
	e=0xf012
	o=0x1fa00000400
	x=0x0
	buf=0x0
	pm=0x41fb80
	fd=0xe
	s=0x41fd20
	j=0x0
_main+0x40 /sys/src/libc/amd64/main9.s:15
acid: 
echo kill > /proc/7624/ctl