New feature: closed tabs trash bin as in Opera

Hello,

---- Intro

I don't remember exactly if I have written anything before
this list, but 
hello everyone in case this is the first time =).

Dani and me are participating in event called "concurso
de software libre" 
(which mean something like "Free Software
Contest") in which students from 
any spanish university can develop a project and in the end
some of them will 
get some prices. Our project was called "Konqueror on
steroids" which was 
about adding some features to the web browser side of
konqueror. 

That was in contrast with the rest of the projects which
choosed to start a 
new app from start, but I felt like I wanted to touch some
KDE 4 code and 
this was a great oportunity to do it.

-- Short Description

We set up an easy first goal and we have already achieved
it: a trahbin with 
recetly closed tabs so that when the user closed a tab he
didn't intend or 
simply wants to retrieve in a fast way one of those tabs he
recently closed, 
he can very well do it. This is idea, as some of the others
we have, came 
from Opera =)

The code is attached to this email. Also, you can get the
latest version of it 
[1] along with the icons you must add to
kdelibs/pics/crystalsvg that Dani 
created for the new action. You can see a screenshot of it
[2]

-- Details

How does it work? easy. It maintains a list of closed URLs
(views) per each 
window, and the first ten elements of that list is showed
when the drop-down 
menu is shown. IF you click in the trash button or select
one element of the 
list, it get's out of there and it gets opened in a new tab.
When there's no 
item in the list, the button gets disabled, etc.

-- Final thoughts

This is a work in progress, and we probably will improve a
bit the thing 
adding the possibility to clean the list, to customize the
number of shown 
items via a configuarition file entry, add numbers and
keystrokes to the 
items in the drop-down menu, maybe we should limit the
number of closed URLs 
to something like 50-100 items...

Or being able to maintain also another list konqueror-wide
with closed windows 
(you know, sometimes shit happens and you close the window
and you want to 
get it back), or taking care not only of the urls but also
of the disposition 
of the views (a tab can have any number of views in many
kinds of 
dispositions). That was just a bit of wild brain storming


What do you think about this? I think it's a valuable
feature that users will 
love (me for one). What are the chances of adding this to
KDE3 branch? (I 
don't think that the backport effort would be difficult).

Thanks for your time,
         Eduardo Robles Elvira.

--
[1] https://forja.rediris.es/websvn/wsvn/csl-konqueror/?sc=0

[2] http://konquerization.wordpress.com/files/2006/12/
trashtab.png

Eduardo Robles Elvira wrote, on Thursday 2006 December 07
9:51 am:
> What do you think about this? I think it's a valuable
feature that users
> will love (me for one). What are the chances of adding
this to KDE3 branch?
> (I don't think that the backport effort would be
difficult).

Hi Eduardo! I'm very happy to see you contributing to KDE!

This *is* a valuable feature that users like me will love.
Instead of a trash 
bin, perhaps it would be more "Consistent" with
KDE to provide an Undo that 
has the same effect? I mean consistent in the sense that you
can undo a 
deletion in KMail in the same way (although it just moves it
in from the 
Trash folder anyway 

Good day,

Charles

El Viernes, 8 de Diciembre de 2006 06:04, Charles Samuels
escribió:
> Hi Eduardo! I'm very happy to see you contributing to
KDE!
>
> This *is* a valuable feature that users like me will
love. Instead of a
> trash bin, perhaps it would be more
"Consistent" with KDE to provide an
> Undo that has the same effect? I mean consistent in the
sense that you can
> undo a deletion in KMail in the same way (although it
just moves it in from
> the Trash folder anyway 

Hello Charles !

We also think that this is somekind of Undo feature.
Actually the trashbin 
icon dani created is a bin with a back arrow on the top of
it .


Konqueror already has somekind of Undo support, and if I
find the way to do it 
I will make this available also through "CTR+Z"
(or whatever keystroke the 
user has assigned to it). That would make it consistent with
KDE.

But that might not be enough. In a web browser we are
opening and closing tabs 
and web pages all the time, because users browse the web
much more than they 
write it. Also, most of the time you want to access last
closed page 
without "undoing" what you writted in current
page. So it becomes very handy 
having a quick list of recetly closed tabs just in case you
want to reopen 
some of them (or just click in the button instead, to get
last closed tab). 
Just food for thought :P

Or perhaps not and this should if anything in the Go menu
and maybe as an 
toolbar button but not enabled by default, because we have
already enough 
buttons in the toolbar =). In opera it's not a normal
toolbar button but a 
small one which apears where we have the "close
tab" mini-button. And in 
Firefox, this appears only in the history menu as
"Recently closed tabs" 
(starting in 2.0).

Have a nice time,
          Eduardo Robles Elvira.

On Friday 08 December 2006 3:04, Eduardo Robles Elvira
wrote:
> small one which apears where we have the "close
tab" mini-button. And in
> Firefox, this appears only in the history menu as
"Recently closed tabs"
> (starting in 2.0).

these two options sound sane, particularly the history menu
approach.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1
A7F1 DB43

Full time KDE developer sponsored by Trolltech (http://www.trolltech.com
)

On Monday 19 February 2007, Eduardo Robles Elvira wrote:
> El Sábado, 17 de Febrero de 2007, escribió:
> > The patch is attached to this email, and in [1]
there 's a tarball with the
> > icon that my fellow team mate created for the
"closedtabs" button. It's a
> > temporal icon that should go inside
kdelibs/pics/crystalsvg. If we change
> > to the new KDE icon theme it probably should get a
new one, but till then
> > we need it.
> >
> > Thepatch is agains kdebase trunk, because it
touches both libkonq and
> > konqueror. 
> 
> Okey, now I have learned a bit more about KConfig and
such things, and now in 
> this new patch there a lekeage (yes! I didn't delete
the KTemporaryFiles I 
> created :P), and now instead of using one new file +
one  new temporary file 
> per closed tab, I use only one KConfig for all of them.
Much more efficient 
> and logical 
> 
> Once more, if you have any comment or suggestion,
insight or you just tested 
> it and liked the feature, don't hesitate to reply
inside this thread.

Some comments from reading the code:
KConfig( "closed_tabs" ... )  => will create a
closed_tabsrc. This should have konqueror in the name
somewhere ;)
Maybe KConfig ("konqueror_closedtabs")?

+ if(!sender()->inherits("KonqUndoManager"))
This breaks if the class is renamed one day, please use
!qt_cast<const KonqUndoManager *>(sender()).
Testing sender() is a bit hackish though, in any case.

m_closedTabsListLength is a constant, so it should become a
file-static imho.

+  m_closedTabsConfig->setGroup( "Closed_Tab" 
+ QString::number(group) );
Please use KConfigGroup grp( m_closedTabsConfig,
"Closed_Tab"  + QString::number(group) ),
and then use grp.readEntry().
setGroup is going away very soon.

+  friend class KonqMainWindow;
Can we avoid that? It's the main user of KonqViewManager
anyway, so the api needed by the mainwindow
can just be made public.


Well done for the integration into konq_undomanager. I have
some questions about that though:
 bool KonqUndoManager::undoAvailable() const
 {
-    return ( d->m_commands.count() > 0 ) &&
!d->m_lock;
+    if(firstClosedTab() > -1)
+    {
+       return true;
+    } else
+       return ( d->m_commands.count() > 0 )
&& !d->m_lock;
 }
Why is this change necessary? "Undo is available if
there's any command to undo" should be general
principle;
and I see that tab-closing is done as a command, as one
might expect. But somehow, tab closing seems
to be have more priority, according to the top of undo().
Why? If I close a tab, then move a file, and then
I press Undo - shouldn't that undo the file moving, instead
of the tab closing?
I think containsAnyClosedTab and firstClosedTab can just go
away...
(Otherwise, the strange operator== could be removed by not
using indexOf, but a simple ++i).

slotAddClosedURL -> slotAddClosedUrl - new naming style
;)

+  QList<ClosedTabItem*> m_closedTabsList;
Memory management might be simpler if using
QList<ClosedTabItem> instead of a pointer.
For instance, what happens if I close 11 tabs? Doesn't the
oldest one get removed from that
list, but would still be in the KonqUndoManager's list, as a
dangling pointer? So in fact
we can't store as value (libkonq doesn't know that class),
but how about making the undo
manager -own- the ClosedTabItem, and then m_closedTabsList
isn't necessary at all?
Of course the signal openLastClosedTab should ship the
ClosedTabItem* to konqueror.
No need to store that data in both places (even as just a
pointer).

Once the konqundomanager api is cleaned up, I would love to
see a few unit tests ;)
But I can also write those in order to check that the
konqundomanager code works as advertised.

-- 
David Faure, faurekde.org, sponsored by Trolltech to work on
KDE,
Konqueror (http://www.konqueror.org
), and KOffice (http://www.koffice.org).

El Lunes, 19 de Febrero de 2007, David Faure escribió:
> Some comments from reading the code:
> KConfig( "closed_tabs" ... )  => will
create a closed_tabsrc. This should
> have konqueror in the name somewhere ;) Maybe KConfig
> ("konqueror_closedtabs")?
>
> +
if(!sender()->inherits("KonqUndoManager"))
> This breaks if the class is renamed one day, please use
!qt_cast<const
> KonqUndoManager *>(sender()). Testing sender() is a
bit hackish though, in
> any case.
>
> m_closedTabsListLength is a constant, so it should
become a file-static
> imho.

okay so far, this sounds alright to me and I'll fix it.

> +  m_closedTabsConfig->setGroup(
"Closed_Tab"  + QString::number(group) );
> Please use KConfigGroup grp( m_closedTabsConfig,
"Closed_Tab"  +
> QString::number(group) ), and then use
grp.readEntry().
> setGroup is going away very soon.

There's a lot of chat over the blogs, IRC  and everywhere
else KDE development 
is present about KConfigGroup, and I also thought this could
be a good idea, 
but I didn't use it because konqueror's functions I used
taked a KConfig as 
an argument. So, I can make it use a KConfigGroup, but I
will need to move 
those functions to the new "standard",
KConfigGroup. I bet that you won't 
stop me for doing this =)

> +  friend class KonqMainWindow;
> Can we avoid that? It's the main user of
KonqViewManager anyway, so the api
> needed by the mainwindow can just be made public.

Actually yes, that's not needed at all as everything needed
is public.

>
> Well done for the integration into konq_undomanager. I
have some questions
> about that though: bool
KonqUndoManager::undoAvailable() const
>  {
> -    return ( d->m_commands.count() > 0 )
&& !d->m_lock;
> +    if(firstClosedTab() > -1)
> +    {
> +       return true;
> +    } else
> +       return ( d->m_commands.count() > 0 )
&& !d->m_lock;
>  }
> Why is this change necessary? "Undo is available
if there's any command to
> undo" should be general principle; and I see that
tab-closing is done as a
> command, as one might expect. But somehow, tab closing
seems to be have
> more priority, according to the top of undo(). Why? If
I close a tab, then
> move a file, and then I press Undo - shouldn't that
undo the file moving,
> instead of the tab closing? I think
containsAnyClosedTab and firstClosedTab
> can just go away... (Otherwise, the strange operator==
could be removed by
> not using indexOf, but a simple ++i).

Okay this leads to an explanation of this changes. I hope
you understand what 
I was trying to achieve and the solution I ended with:

Okay this is what happens: when Konqueror is in KHTML mode,
m_lock is always 
off and it's not possible to execute "normal" undo
actions. Read "normal" 
as "current", like "File renamed" or
"Moved file". And I understand that the 
purpose of m_lock is precisely to block those when we're in
a not editable 
mode, like when the active view is showing a webpage.

But now there's a new kind of actions in the undo list: one
that 
doesn't "edit" the content managed by the views.
It's the "Closed tab" 
action, which is of a different kind. It's an action that
should be able to 
be undone (or however it's said :P) in any kind of view.

So in this case, I treat the "closed tab" action
as a normal one when the 
current view is editable, but in the non-editable views
(like KHTML) we still 
want to be able to reopen all previously closed tabs, so
undoAvailable() 
returns true if there's any closed tab, and it only allows
to undo one kind 
of actions of the undo stack: "closed tabs".

That's it!

> slotAddClosedURL -> slotAddClosedUrl - new naming
style ;)

Oh yeah, my fault sometimes this kind of thing happens hehe
thanks fore 
pointing it out.
>
> +  QList<ClosedTabItem*> m_closedTabsList;
> Memory management might be simpler if using
QList<ClosedTabItem> instead of
> a pointer. For instance, what happens if I close 11
tabs? Doesn't the
> oldest one get removed from that list, but would still
be in the
> KonqUndoManager's list, as a dangling pointer? So in
fact we can't store as
> value (libkonq doesn't know that class), but how about
making the undo
> manager -own- the ClosedTabItem, and then
m_closedTabsList isn't necessary
> at all? Of course the signal openLastClosedTab should
ship the
> ClosedTabItem* to konqueror. No need to store that data
in both places
> (even as just a pointer).

As suggested by ereslibre, I didn't limit the number of
stored tabs because 
they don't take much memory, I only limited the number of
them that are shown 
in the drop down menu. So if you close 11 tabs, all of them
will be stored in 
m_closedTabsList, but only the last 10 of them will be shown
in the "Recently 
closed tabs" menu. That's why I use a list of pointers
instead of an array or 
a lis of values: I need to dynamically create and remove eah
item with 
new/delete. And also I belive of this there shouldn't be any
dangling 
pointer.

It's true is that we could put everything in the undo list
so we don't have 
two lists, that's an idea I didn't take into account, and it
 will take less 
space in memory, the only problem will be that 
slotClosedTabsListAboutToShow() will then call a function in
the undomanager 
that would run through all undoActions and retrieve a list
of 
ClosedTabItem*s. I'm not sure if given that we need pointers
anyway you still 
think this is desirable. I suppose yes, but I still want you
to tell me =)

> Once the konqundomanager api is cleaned up, I would
love to see a few unit
> tests ;) But I can also write those in order to check
that the
> konqundomanager code works as advertised.

Everyone loves units tests, maybe I can make some of them
and learn to use the 
unit test framework along the way.

Thanks for your time,
          Eduardo Robles Elvira.

El Lunes, 19 de Febrero de 2007, David Faure escribió:
> KConfig( "closed_tabs" ... )  => will
create a closed_tabsrc. This should
> have konqueror in the name somewhere ;) Maybe KConfig
> ("konqueror_closedtabs")?

Fixed

> +
if(!sender()->inherits("KonqUndoManager"))
> This breaks if the class is renamed one day, please use
!qt_cast<const
> KonqUndoManager *>(sender()). Testing sender() is a
bit hackish though, in
> any case.

Fixed, now it doesn't test the sender anymore

> m_closedTabsListLength is a constant, so it should
become a file-static
> imho.

Done

> +  m_closedTabsConfig->setGroup(
"Closed_Tab"  + QString::number(group) );
> Please use KConfigGroup grp( m_closedTabsConfig,
"Closed_Tab"  +
> QString::number(group) ), and then use
grp.readEntry().
> setGroup is going away very soon.

Okey this is the only thing I didn't fix. Using a
KConfigGroup required major 
changes in konqueror code, probably including plugins and
such. Once 
KonqFrameBase::saveConfig and KonqFrameBase::loadItem end up
taking a 
KConfigGroup instead of a KConfig as an argument, I promise
I will stop using 
setGroup.

> +  friend class KonqMainWindow;
> Can we avoid that? It's the main user of
KonqViewManager anyway, so the api
> needed by the mainwindow can just be made public.

Done, friend statement removed

> Well done for the integration into konq_undomanager. I
have some questions
> about that though: bool
KonqUndoManager::undoAvailable() const
>  {
> -    return ( d->m_commands.count() > 0 )
&& !d->m_lock;
> +    if(firstClosedTab() > -1)
> +    {
> +       return true;
> +    } else
> +       return ( d->m_commands.count() > 0 )
&& !d->m_lock;
>  }
> Why is this change necessary? "Undo is available
if there's any command to
> undo" should be general principle; and I see that
tab-closing is done as a
> command, as one might expect. But somehow, tab closing
seems to be have
> more priority, according to the top of undo(). Why? If
I close a tab, then
> move a file, and then I press Undo - shouldn't that
undo the file moving,
> instead of the tab closing? I think
containsAnyClosedTab and firstClosedTab
> can just go away... (Otherwise, the strange operator==
could be removed by
> not using indexOf, but a simple ++i).

As I said in the last email, this is necessary. But
operator== have been 
removed 

> slotAddClosedURL -> slotAddClosedUrl - new naming
style ;)

Done

> +  QList<ClosedTabItem*> m_closedTabsList;
> Memory management might be simpler if using
QList<ClosedTabItem> instead of
> a pointer. For instance, what happens if I close 11
tabs? Doesn't the
> oldest one get removed from that list, but would still
be in the
> KonqUndoManager's list, as a dangling pointer? So in
fact we can't store as
> value (libkonq doesn't know that class), but how about
making the undo
> manager -own- the ClosedTabItem, and then
m_closedTabsList isn't necessary
> at all? Of course the signal openLastClosedTab should
ship the
> ClosedTabItem* to konqueror. No need to store that data
in both places
> (even as just a pointer).

Done (probably it was the most difficult part 
Now there's no m_closedTabsList anymore, every closedTab is
only listed in the 
UndoManager.

Probably you have still some questions, if so I will answer
and try to correct 
any problem with the code.

        Eduardo Robles Elvira.