Re: [PATCH] awful.tag: fix tag history

[koniu]

On Fri, Aug 28, 2009 at 09:57, Julien Danjoujul...@danjou.info wrote:

 +        if not data.history[screen] then
 +          data.history[screen] = {}
 Bad indent.

Fixed.

 +        -- limit history
 +        elseif #data.history[screen] == history.limit then
 +            table.remove(data.history[screen])
 Be careful that the limit might have been reduced to let's say 20 to 10.
 So you won't hit it with a ==, you should do a while  history, remove
 lines I guess.

That's probably not gonna be a very common case but better be safe
than sorry - fixed.

 +        data.history.current[screen] = selectedlist(screen)
 Hum if you want to store tags object I'm ok with that but you should
 probably use a weak-value table in that case. And be careful if tags are
 removed from the screen and/or collected by the GC, since your array
 will be (partially) empty.

That's what I was thinking especially being a shifty-man where tags
come and go. Should be fixed now.

 +function history.restore(screen, idx)
 I think that should be split in 2 functions. Add another function like
 goback that goes back N elements in history for example, but leave
 restore as it is, e.g. only switch between 2 elements.

I'll allow myself to disagree, splitting it would result in pointless
code duplication whereas this approach makes certain things easier
(eg. falling back onto further history entries when restoring to
'previous') etc. 'previous' is, for me, a just a special case of this
function so making it 2 separate functions seems like an overkill.

Can you give some solid arguments for splitting it?

 Otherwise, the code actually works. :)

And even better now, see attached patches. And yeah - sorry for the
delay - turned out to be a busy weekend ;].

koniu
From 0052beccbb7e6c3f95cec5ff40c5c07ddffa5652 Mon Sep 17 00:00:00 2001
From: koniu gkusni...@gmail.com
Date: Thu, 27 Aug 2009 15:03:45 +0100
Subject: [PATCH 1/3] awful.tag: fix and improve tag history

This fixes a long standing tag history breakage. To store history
of tag switching we rely on a special signal tag::history::update
which needs to be emitted by any function which deals with tag
selection.

History is multi-level with a configurable limit:
awful.tag.history.limit = 20 (by default).

awful.tag.history.restore function gets a new argument 'idx' which can
be either 'previous' (default) which will revert to the previously
selected set of tags, or a numerical index in the tag history table.

Signed-off-by: koniu gkusni...@gmail.com
---
 lib/awful/tag.lua.in |   87 --
 1 files changed, 49 insertions(+), 38 deletions(-)

diff --git a/lib/awful/tag.lua.in b/lib/awful/tag.lua.in
index c54066d..1fe1706 100644
--- a/lib/awful/tag.lua.in
+++ b/lib/awful/tag.lua.in
@@ -24,37 +24,11 @@ module(awful.tag)
 -- Private data
 local data = {}
 data.history = {}
-data.history.past = {}
-data.history.current = {}
 data.tags = setmetatable({}, { __mode = 'k' })
 
 -- History functions
 history = {}
-
--- Compare 2 tables of tags.
--- @param a The first table.
--- @param b The second table of tags.
--- @return True if the tables are identical, false otherwise.
-local function compare_select(a, b)
-if not a or not b then
-return false
-end
--- Quick size comparison
-if #a ~= #b then
-return false
-end
-for ka, va in pairs(a) do
-if b[ka] ~= va.selected then
-return false
-end
-end
-for kb, vb in pairs(b) do
-if a[kb].selected ~= vb then
-return false
-end
-end
-return true
-end
+history.limit = 20
 
 --- Create a set of tags and attach it to a screen.
 -- @param names The tag name, in a table
@@ -77,26 +51,56 @@ function new(names, screen, layout)
 end
 
 --- Update the tag history.
--- @param screen The screen number.
-function history.update(screen)
-local curtags = capi.screen[screen]:tags()
-if not compare_select(curtags, data.history.current[screen]) then
-data.history.past[screen] = data.history.current[screen]
-data.history.current[screen] = {}
-for k, v in ipairs(curtags) do
-data.history.current[screen][k] = v.selected
+-- @param obj Screen object.
+function history.update(obj)
+local s = obj.index
+local curtags = capi.screen[s]:tags()
+-- create history table
+if not data.history[s] then
+data.history[s] = {}
+-- limit history
+elseif #data.history[s] = history.limit then
+for i = history.limit, #data.history[s] do
+data.history[s][i] = nil
 end
 end
+-- store previously selected tags in the history table
+table.insert(data.history[s], 1, data.history[s].current)
+data.history[s].previous = data.history[s][1]
+-- store currently selected tags
+data.history[s].current = setmetatable(selectedlist(s), { __mode = 'v' })
 end
 
 --- Revert tag history.
 -- @param screen The screen number.
-function 

Re: [PATCH] awful.tag: fix tag history

[Julien Danjou]

At 1251393183 time_t, koniu wrote:
 +if not compare_select(curtags, data.history.current[screen]) and 
 #selectedlist(screen)  0 then
 +-- create history table
 +if not data.history[screen] then
 +  data.history[screen] = {}

Bad indent.

 +-- limit history
 +elseif #data.history[screen] == history.limit then
 +table.remove(data.history[screen])
  end

Be careful that the limit might have been reduced to let's say 20 to 10.
So you won't hit it with a ==, you should do a while  history, remove
lines I guess.

 +-- store previously selected tags in the history table
 +table.insert(data.history[screen], 1, data.history.current[screen])
 +data.history[screen].previous = data.history[screen][1]
 +-- store currently selected tags
 +data.history.current[screen] = selectedlist(screen)

Hum if you want to store tags object I'm ok with that but you should
probably use a weak-value table in that case. And be careful if tags are
removed from the screen and/or collected by the GC, since your array
will be (partially) empty.

  --- Revert tag history.
  -- @param screen The screen number.
 -function history.restore(screen)
 +-- @param idx Index in history. Defaults to previous which is a special 
 index
 +-- toggling between last two selected sets of tags. Number (eg 1) will go 
 back
 +-- to the given index in history.
 +function history.restore(screen, idx)
  local s = screen or capi.mouse.screen
  local tags = capi.screen[s]:tags()
 -for k, t in pairs(tags) do
 -t.selected = data.history.past[s][k]
 +local i = idx or previous
 +local previous = selectedlist(s)
 +-- do nothing if history empty
 +if not data.history[s] or not data.history[s][i] then return end
 +-- deselect all tags
 +viewnone(s)
 +-- select tags from the history entry
 +for _, t in ipairs(data.history[s][i]) do
 +t.selected = true
  end
 +-- update currently selected tags table
 +data.history.current[s] = data.history[s][i]
 +-- store previously selected tags
 +data.history[s][previous] = previous
 +-- remove the reverted history entry
 +if i ~= previous then table.remove(data.history[s], i) end
  end

I think that should be split in 2 functions. Add another function like
goback that goes back N elements in history for example, but leave
restore as it is, e.g. only switch between 2 elements.

Otherwise, the code actually works. :)

Cheers,
-- 
Julien Danjou
// ᐰ jul...@danjou.info   http://julien.danjou.info
// 9A0D 5FD9 EB42 22F6 8974  C95C A462 B51E C2FE E5CD
// Don't give up.


signature.asc
Description: Digital signature

Re: [PATCH] awful.tag: fix tag history

[koniu]

 W: awesome: luaA_dofunction:113: error while running function
 stack traceback:
        /usr/local/share/awesome/lib/awful/tag.lua:107: in function 'press'
        /usr/local/share/awesome/lib/awful/key.lua:36: in function 
 /usr/local/share/awesome/lib/awful/key.lua:36
 error: /usr/local/share/awesome/lib/awful/tag.lua:107: table index is nil

 Sometimes meaning right after reload or after repeated restore until
 you run out of history entries? Indeed, I didnt add handling of
 restore when there's nothing recorded.

Actually this is different issue, L107:s/screen/s/ ? Try the amended
patch as attached.
From 067a6ea8ddfe4b5a302a29fe479018f79dc833fd Mon Sep 17 00:00:00 2001
From: koniu gkusni...@gmail.com
Date: Thu, 27 Aug 2009 15:03:45 +0100
Subject: [PATCH 1/3] awful.tag: fix tag history

This fixes a long standing tag history breakage. To store history
of tag switching we rely on a special signal tag::history::update
which needs to be emitted by any function which deals with tag
selection.

History is now also multi-level with a 20 step limit.

Signed-off-by: koniu gkusni...@gmail.com
---
 lib/awful/tag.lua.in |   36 ++--
 1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/lib/awful/tag.lua.in b/lib/awful/tag.lua.in
index 8ec1bbd..980e008 100644
--- a/lib/awful/tag.lua.in
+++ b/lib/awful/tag.lua.in
@@ -24,7 +24,6 @@ module(awful.tag)
 -- Private data
 local data = {}
 data.history = {}
-data.history.past = {}
 data.history.current = {}
 data.tags = setmetatable({}, { __mode = 'k' })
 
@@ -77,15 +76,22 @@ function new(names, screen, layout)
 end
 
 --- Update the tag history.
--- @param screen The screen number.
-function history.update(screen)
+-- @param obj Screen object.
+function history.update(obj)
+local screen = obj.index
 local curtags = capi.screen[screen]:tags()
-if not compare_select(curtags, data.history.current[screen]) then
-data.history.past[screen] = data.history.current[screen]
-data.history.current[screen] = {}
-for k, v in ipairs(curtags) do
-data.history.current[screen][k] = v.selected
+if not compare_select(curtags, data.history.current[screen]) and #selectedlist(screen)  0 then
+-- create history table
+if not data.history[screen] then
+  data.history[screen] = {} 
+-- limit history to 20 steps
+elseif #data.history[screen] == 20 then
+table.remove(data.history[screen])
 end
+-- store previously selected tags in the history table
+table.insert(data.history[screen], 1, data.history.current[screen])
+-- store currently selected tags
+data.history.current[screen] = selectedlist(screen)
 end
 end
 
@@ -94,9 +100,12 @@ end
 function history.restore(screen)
 local s = screen or capi.mouse.screen
 local tags = capi.screen[s]:tags()
-for k, t in pairs(tags) do
-t.selected = data.history.past[s][k]
+viewnone(s)
+for _, t in ipairs(data.history[s][1]) do
+t.selected = true
 end
+data.history.current[s] = data.history[s][1]
+table.remove(data.history[s], 1)
 end
 
 --- Return a table with all visible tags
@@ -231,6 +240,7 @@ function viewidx(i, screen)
 showntags[util.cycle(#showntags, k + i)].selected = true
 end
 end
+capi.screen[screen]:emit_signal(tag::history::update)
 end
 
 --- View next tag. This is the same as tag.viewidx(1).
@@ -250,6 +260,7 @@ end
 function viewonly(t)
 viewnone(t.screen)
 t.selected = true
+capi.screen[t.screen]:emit_signal(tag::history::update)
 end
 
 --- View only a set of tags.
@@ -260,6 +271,7 @@ function viewmore(tags, screen)
 for i, t in pairs(tags) do
 t.selected = true
 end
+capi.screen[screen]:emit_signal(tag::history::update)
 end
 
 --- Get tag data table.
@@ -339,6 +351,10 @@ capi.client.add_signal(manage, function(c)
 c:add_signal(property::screen, withcurrent)
 end)
 
+for s = 1, capi.screen.count() do
+capi.screen[s]:add_signal(tag::history::update, history.update)
+end
+
 setmetatable(_M, { __call = function (_, ...) return new(...) end })
 
 -- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:encoding=utf-8:textwidth=80
-- 
1.6.3.3

Re: [PATCH] awful.tag: fix tag history

[Julien Danjou]

At 1251387477 time_t, koniu wrote:
 Sometimes meaning right after reload or after repeated restore until
 you run out of history entries? Indeed, I didnt add handling of
 restore when there's nothing recorded.

No, I don't think so. I'm using it like a dumb user:

I switch to tag 1;
I switch to tag 2;
I switch to tag 3;
I press Mod4+Escape;
it goes back to tag 2;
I press Mod4+Escape;
it yells at me what I've pasted. :)

-- 
Julien Danjou
// ᐰ jul...@danjou.info   http://julien.danjou.info
// 9A0D 5FD9 EB42 22F6 8974  C95C A462 B51E C2FE E5CD
// My root password is


signature.asc
Description: Digital signature

Re: [PATCH] awful.tag: fix tag history

[Julien Danjou]

At 1251387674 time_t, koniu wrote:
 Actually this is different issue, L107:s/screen/s/ ? Try the amended
 patch as attached.

Seems better indeed!

Seems ok to merge, except that there's still 3 things that I'd like:
- Configurable history limit (20 is arbitrary)
  a variable in the module that the user can modify is enough and
  trivial to add;
- The old behaviour was fine for restore(), this one is totally
  different since it always goes -1. This can be another
  feature/keybinding but I think the old one should be restored;
- Handling of out of history not yelling errors. :)

Thanks koniu.

Cheers,
-- 
Julien Danjou
// ᐰ jul...@danjou.info   http://julien.danjou.info
// 9A0D 5FD9 EB42 22F6 8974  C95C A462 B51E C2FE E5CD
// My root password is


signature.asc
Description: Digital signature

Re: [PATCH] awful.tag: fix tag history

[koniu]

A little amendment to the 1st patch - typo in a comment + dirty whitespace.

k
From 275eeb31ab4e2e9badc63cbcf637efff3b0c8a8b Mon Sep 17 00:00:00 2001
From: koniu gkusni...@gmail.com
Date: Thu, 27 Aug 2009 15:03:45 +0100
Subject: [PATCH 1/4] awful.tag: fix and improve tag history

This fixes a long standing tag history breakage. To store history
of tag switching we rely on a special signal tag::history::update
which needs to be emitted by any function which deals with tag
selection.

History is multi-level with a configurable limit:
awful.tag.history.limit = 20 (by default).

awful.tag.history.restore function gets a new argument 'idx' which can
be either 'previous' (default) which will revert to the previously
selected set of tags, or a numerical index in the tag history table.

Signed-off-by: koniu gkusni...@gmail.com
---
 lib/awful/tag.lua.in |   53 +++--
 1 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/lib/awful/tag.lua.in b/lib/awful/tag.lua.in
index 8ec1bbd..be316c4 100644
--- a/lib/awful/tag.lua.in
+++ b/lib/awful/tag.lua.in
@@ -24,12 +24,12 @@ module(awful.tag)
 -- Private data
 local data = {}
 data.history = {}
-data.history.past = {}
 data.history.current = {}
 data.tags = setmetatable({}, { __mode = 'k' })
 
 -- History functions
 history = {}
+history.limit = 20
 
 -- Compare 2 tables of tags.
 -- @param a The first table.
@@ -77,26 +77,50 @@ function new(names, screen, layout)
 end
 
 --- Update the tag history.
--- @param screen The screen number.
-function history.update(screen)
+-- @param obj Screen object.
+function history.update(obj)
+local screen = obj.index
 local curtags = capi.screen[screen]:tags()
-if not compare_select(curtags, data.history.current[screen]) then
-data.history.past[screen] = data.history.current[screen]
-data.history.current[screen] = {}
-for k, v in ipairs(curtags) do
-data.history.current[screen][k] = v.selected
+if not compare_select(curtags, data.history.current[screen]) and #selectedlist(screen)  0 then
+-- create history table
+if not data.history[screen] then
+  data.history[screen] = {}
+-- limit history
+elseif #data.history[screen] == history.limit then
+table.remove(data.history[screen])
 end
+-- store previously selected tags in the history table
+table.insert(data.history[screen], 1, data.history.current[screen])
+data.history[screen].previous = data.history[screen][1]
+-- store currently selected tags
+data.history.current[screen] = selectedlist(screen)
 end
 end
 
 --- Revert tag history.
 -- @param screen The screen number.
-function history.restore(screen)
+-- @param idx Index in history. Defaults to previous which is a special index
+-- toggling between last two selected sets of tags. Number (eg 1) will go back
+-- to the given index in history.
+function history.restore(screen, idx)
 local s = screen or capi.mouse.screen
 local tags = capi.screen[s]:tags()
-for k, t in pairs(tags) do
-t.selected = data.history.past[s][k]
+local i = idx or previous
+local previous = selectedlist(s)
+-- do nothing if history empty
+if not data.history[s] or not data.history[s][i] then return end
+-- deselect all tags
+viewnone(s)
+-- select tags from the history entry
+for _, t in ipairs(data.history[s][i]) do
+t.selected = true
 end
+-- update currently selected tags table
+data.history.current[s] = data.history[s][i]
+-- store previously selected tags
+data.history[s][previous] = previous
+-- remove the reverted history entry
+if i ~= previous then table.remove(data.history[s], i) end
 end
 
 --- Return a table with all visible tags
@@ -231,6 +255,7 @@ function viewidx(i, screen)
 showntags[util.cycle(#showntags, k + i)].selected = true
 end
 end
+capi.screen[screen]:emit_signal(tag::history::update)
 end
 
 --- View next tag. This is the same as tag.viewidx(1).
@@ -250,6 +275,7 @@ end
 function viewonly(t)
 viewnone(t.screen)
 t.selected = true
+capi.screen[t.screen]:emit_signal(tag::history::update)
 end
 
 --- View only a set of tags.
@@ -260,6 +286,7 @@ function viewmore(tags, screen)
 for i, t in pairs(tags) do
 t.selected = true
 end
+capi.screen[screen]:emit_signal(tag::history::update)
 end
 
 --- Get tag data table.
@@ -339,6 +366,10 @@ capi.client.add_signal(manage, function(c)
 c:add_signal(property::screen, withcurrent)
 end)
 
+for s = 1, capi.screen.count() do
+capi.screen[s]:add_signal(tag::history::update, history.update)
+end
+
 setmetatable(_M, { __call = function (_, ...) return new(...) end })
 
 -- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:encoding=utf-8:textwidth=80
-- 
1.6.3.3