[PATCH] window.el: Improve mark management

[PATCH] window.el: Improve mark management

[Jérémy Compostella]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

All,

I think the current window.el `window-state-get' and `window-state-put'
function does not manage the mark well.

First, `mark-active' save was missing in the `window-state-get'
function.  Second, `window-state-put' restore the mark without taking
into account the previously set mark(s). It looks better to use
`push-mark' instead of `set-mark'. Moreover it does not restore the
active mark state which lead to an automatically active mark in
restored buffer.

Please, review it or merge it,

Best Regards,

Jérémy


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] window.el: Improve mark management --]
[-- Type: text/x-diff, Size: 1923 bytes --]

From d50e46f53dd09f9ab133ef3ca048ec3cd8023bdf Mon Sep 17 00:00:00 2001
From: Jeremy Compostella <jeremy.compostella@gmail.com>
Date: Thu, 19 Jan 2012 20:21:58 +0100
Subject: [PATCH] window.el: Improve mark management

First, `mark-active' save was missing in the `window-state-get'
function.  Second, `window-state-put' restore the mark without taking
into account the previously set mark(s). It looks better to use
`push-mark' instead of `set-mark'. Moreover it does not restore the
active mark state which lead to an automatically active mark in
restored buffer.

Signed-off-by: Jeremy Compostella <jeremy.compostella@gmail.com>
---
 lisp/window.el |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index 54e5ec9..6a2be23 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -3632,7 +3632,8 @@ specific buffers."
                        (start . ,(if ignore start (copy-marker start)))
                        ,@(when mark
                            `((mark . ,(if ignore
-                                          mark (copy-marker mark))))))))))))
+                                          mark (copy-marker mark)))
+			     (mark-active . ,mark-active))))))))))
 	 (tail
 	  (when (memq type '(vc hc))
 	    (let (list)
@@ -3816,10 +3817,10 @@ value can be also stored on disk and read back in a new session."
 	  (ignore-errors
 	    (set-window-start window (cdr (assq 'start state)))
 	    (set-window-point window (cdr (assq 'point state)))
-	    ;; I'm not sure whether we should set the mark here, but maybe
-	    ;; it can be used.
 	    (let ((mark (cdr (assq 'mark state))))
-	      (when mark (set-mark mark))))
+	      (when mark
+		(push-mark mark t)
+		(setq mark-active (cdr (assq 'mark-active state))))))
 	  ;; Select window if it's the selected one.
 	  (when (cdr (assq 'selected state))
 	    (select-window window)))))))
-- 
1.7.2.5

Re: [PATCH] window.el: Improve mark management

[martin rudalics]

 > First, `mark-active' save was missing in the `window-state-get'
 > function.  Second, `window-state-put' restore the mark without taking
 > into account the previously set mark(s). It looks better to use
 > `push-mark' instead of `set-mark'. Moreover it does not restore the
 > active mark state which lead to an automatically active mark in
 > restored buffer.
 >
 > Please, review it or merge it,

Looks good to me.

I shall install this in a couple of days if no one objects.

Thanks, martin

Re: [PATCH] window.el: Improve mark management

[Stefan Monnier]

> into account the previously set mark(s). It looks better to use
> `push-mark' instead of `set-mark'. Moreover it does not restore the
> active mark state which lead to an automatically active mark in
> restored buffer.

`push-mark' is wrong if nothing happened between window-state-get and
window-state-put.
And the mark & mark-active are buffer-local but not window-local.
If we window-state-get in a frame which shows the same buffer several
times, window-state-put would end up pushing the same mark several times.


        Stefan

Re: [PATCH] window.el: Improve mark management

[martin rudalics]

 > `push-mark' is wrong if nothing happened between window-state-get and
 > window-state-put.
 > And the mark & mark-active are buffer-local but not window-local.
 > If we window-state-get in a frame which shows the same buffer several
 > times, window-state-put would end up pushing the same mark several times.

We could refuse pushing the mark when current and old position are the
same.  But obviously pushing the mark from a non-current buffer is not
very reasonable either.

IIRC all I tried was to emulate the behavior for window configurations.
What would you suggest instead?  Not save the mark in the first place?

martin

Re: [PATCH] window.el: Improve mark management

[Stefan Monnier]

>> `push-mark' is wrong if nothing happened between window-state-get and
>> window-state-put.
>> And the mark & mark-active are buffer-local but not window-local.
>> If we window-state-get in a frame which shows the same buffer several
>> times, window-state-put would end up pushing the same mark several times.

> We could refuse pushing the mark when current and old position are the
> same.  But obviously pushing the mark from a non-current buffer is not
> very reasonable either.

> IIRC all I tried was to emulate the behavior for window configurations.
> What would you suggest instead?  Not save the mark in the first place?

I'm not completely sure.  The current code doesn't seem that bad since,
as you say, it basically reproduces the behavior of
window-configurations.

So I'd like to first hear of what are concrete cases where the current
behavior is a problem.


        Stefan

Re: [PATCH] window.el: Improve mark management

[Jérémy Compostella]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

2012/1/20 Stefan Monnier <monnier@iro.umontreal.ca>

> > into account the previously set mark(s). It looks better to use
> > `push-mark' instead of `set-mark'. Moreover it does not restore the
> > active mark state which lead to an automatically active mark in
> > restored buffer.
>
> `push-mark' is wrong if nothing happened between window-state-get and
> window-state-put.
>
I do agree but the use of set-mark leads to the lost of the potential
previous mark.
Maybe I could just add a control like call `push-mark' only if current if
current mark
is not equal to the saved one.


> And the mark & mark-active are buffer-local but not window-local.
> If we window-state-get in a frame which shows the same buffer several
> times, window-state-put would end up pushing the same mark several times.

That was more of less the subject of  my previous thread ("[PATCH]
window.el: Remove mark saving and restoring")
that I cancel to have time to think about it a little bit more. The
proposition was to remove the mark
stuff (I did provide a patch for this) that looks not relevant in the
`window-state-get' and
`window-state-put'. However but thinking more about it I figured out that
somebody could want the mark restored.

> I'm not completely sure.  The current code doesn't seem that bad since,
> as you say, it basically reproduces the behavior of
> window-configurations.
> So I'd like to first hear of what are concrete cases where the current
> behavior is a problem.
It's very simple : with the current code, when you call the
`window-state-put' function, the mark is restored as active.

In conclusion I think we have to make a choice:
- Either, we accept the idea that the mark is restored and we should take
care of its active state, avoid the previous
potential mark loss and do not push-mark when the current `mark' is equal
to the saved one.
- Either, remove all the mark stuff from `window-state-get' and
`window-state-put' functions.
- Other ?

I have patch almost ready for these two proposals so let me know.

Jérémy

[-- Attachment #2: Type: text/html, Size: 2936 bytes --]

Re: [PATCH] window.el: Improve mark management

[Stefan Monnier]

>> I'm not completely sure.  The current code doesn't seem that bad since,
>> as you say, it basically reproduces the behavior of
>> window-configurations.
>> So I'd like to first hear of what are concrete cases where the current
>> behavior is a problem.
> It's very simple : with the current code, when you call the
> `window-state-put' function, the mark is restored as active.

That's easy to fix: use (set-marker (mark-marker)) instead of set-mark.

> - Either, remove all the mark stuff from `window-state-get' and
> `window-state-put' functions.

That's not a bad idea, actually.


        Stefan


PS: BTW, the "multiple push-mark" issue can be solved by saving the mark
only once per buffer rather than once per window (i.e. the window-state
object would have a separate list of affected buffers with their
corresponding state, including the mark, not sure what else could be
there).
        

Re: [PATCH] window.el: Improve mark management

[Jérémy Compostella]

[-- Attachment #1.1: Type: text/plain, Size: 1502 bytes --]

2012/1/20 Stefan Monnier <monnier@iro.umontreal.ca>

> >> I'm not completely sure.  The current code doesn't seem that bad since,
> >> as you say, it basically reproduces the behavior of
> >> window-configurations.
> >> So I'd like to first hear of what are concrete cases where the current
> >> behavior is a problem.
> > It's very simple : with the current code, when you call the
> > `window-state-put' function, the mark is restored as active.
>
> That's easy to fix: use (set-marker (mark-marker)) instead of set-mark.
>
Interesting.

>
> > - Either, remove all the mark stuff from `window-state-get' and
> > `window-state-put' functions.
>
> That's not a bad idea, actually.
>
Yes that's was my first idea in fact. After this discussion I think it's
the good
thing to do and ou I attached the corresponding patch to this email.
`window-state-get' and `window-state-put' should not deal with mark as it's
not
relevant at all and leads to an unexpected behavior from user point of view.


>
>        Stefan
>
>
> PS: BTW, the "multiple push-mark" issue can be solved by saving the mark
> only once per buffer rather than once per window (i.e. the window-state
> object would have a separate list of affected buffers with their
> corresponding state, including the mark, not sure what else could be
> there).
>
I do agree but it will change several things and while we are in feature
freeze I
propose to remove the wrong mark management and see how to do it this way
in future development.

Thanks.

[-- Attachment #1.2: Type: text/html, Size: 2329 bytes --]

[-- Attachment #2: 0001-window.el-Remove-mark-save-and-restore.patch --]
[-- Type: text/x-patch, Size: 2046 bytes --]

From e517319ee1d5fbb8789fdced96dde8d212efd1b2 Mon Sep 17 00:00:00 2001
From: Jeremy Compostella <jeremy.compostella@gmail.com>
Date: Thu, 19 Jan 2012 14:33:32 +0100
Subject: [PATCH] window.el: Remove mark save and restore

As long as I know mark is related to a buffer and not to a window. It should
be saved by buffer functions and not window functions. Moreover, when
I call window-state-put on a window-state-get previously stored the
mark is changed and activated which does not look like the desired behavior.

Signed-off-by: Jeremy Compostella <jeremy.compostella@gmail.com>
---
 lisp/window.el |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index 9122904..832a08d 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -3622,10 +3622,7 @@ specific buffers."
                        (vscroll . ,(window-vscroll window))
                        (dedicated . ,(window-dedicated-p window))
                        (point . ,(if writable point (copy-marker point)))
-                       (start . ,(if writable start (copy-marker start)))
-                       ,@(when mark
-                           `((mark . ,(if writable
-                                          mark (copy-marker mark))))))))))))
+                       (start . ,(if writable start (copy-marker start))))))))))
 	 (tail
 	  (when (memq type '(vc hc))
 	    (let (list)
@@ -3809,11 +3806,7 @@ value can be also stored on disk and read back in a new session."
 	  ;; have been created and sized).
 	  (ignore-errors
 	    (set-window-start window (cdr (assq 'start state)))
-	    (set-window-point window (cdr (assq 'point state)))
-	    ;; I'm not sure whether we should set the mark here, but maybe
-	    ;; it can be used.
-	    (let ((mark (cdr (assq 'mark state))))
-	      (when mark (set-mark mark))))
+	    (set-window-point window (cdr (assq 'point state))))
 	  ;; Select window if it's the selected one.
 	  (when (cdr (assq 'selected state))
 	    (select-window window)))))))
-- 
1.7.2.5

Re: [PATCH] window.el: Improve mark management

[Stefan Monnier]

>> >> I'm not completely sure.  The current code doesn't seem that bad since,
>> >> as you say, it basically reproduces the behavior of
>> >> window-configurations.
>> >> So I'd like to first hear of what are concrete cases where the current
>> >> behavior is a problem.
>> > It's very simple : with the current code, when you call the
>> > `window-state-put' function, the mark is restored as active.
>> That's easy to fix: use (set-marker (mark-marker)) instead of set-mark.
> Interesting.

It's actually a trivial bug-fix.

>> > - Either, remove all the mark stuff from `window-state-get' and
>> > `window-state-put' functions.
>> That's not a bad idea, actually.
> Yes that's was my first idea in fact. After this discussion I think
> it's the good thing to do and ou I attached the corresponding patch to
> this email.  `window-state-get' and `window-state-put' should not deal
> with mark as it's not relevant at all and leads to an unexpected
> behavior from user point of view.

I tend to agree.  Martin, what do you think?


        Stefan

Re: [PATCH] window.el: Improve mark management

[martin rudalics]

 >> `window-state-get' and `window-state-put' should not deal
 >> with mark as it's not relevant at all and leads to an unexpected
 >> behavior from user point of view.
 >
 > I tend to agree.  Martin, what do you think?

I agree as well.  I'll also remove the parts dealing with
`window-size-fixed'.

martin

Re: [PATCH] window.el: Improve mark management

[Stefan Monnier]

>>> `window-state-get' and `window-state-put' should not deal
>>> with mark as it's not relevant at all and leads to an unexpected
>>> behavior from user point of view.
>> I tend to agree.  Martin, what do you think?
> I agree as well.  I'll also remove the parts dealing with
> `window-size-fixed'.

Great, thank you,


        Stefan

Re: [PATCH] window.el: Improve mark management

[Jérémy Compostella]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

Thank you to both of you.

Jérémy

2012/1/25 Stefan Monnier <monnier@iro.umontreal.ca>

> >>> `window-state-get' and `window-state-put' should not deal
> >>> with mark as it's not relevant at all and leads to an unexpected
> >>> behavior from user point of view.
> >> I tend to agree.  Martin, what do you think?
> > I agree as well.  I'll also remove the parts dealing with
> > `window-size-fixed'.
>
> Great, thank you,
>
>
>        Stefan
>



-- 
« Si debugger, c'est supprimer des bugs, alors programmer ne peut être que
les ajouter » - Edsger Dijkstra

[-- Attachment #2: Type: text/html, Size: 1003 bytes --]