Shared undo closed windows list over D-Bus between different konqueror processes

[David Faure]

On Sunday 23 March 2008, Eduardo Robles Elvira wrote:
> Hi all!
> 
> Finally it's done, after two days working hard on this. I think this D-Bus 
> patch is complete, I've tested it for a while and seems to be working well.
> 
> I attach the patch here so that you can make some comments before commiting. 
> There are some code decissions I had to do that might be difficult to explain 
> but I will explain them happily if asked.

Can you explain the refcounting? What is being refcounted? It seems to be the number of konqueror 
windows, from all running processes, and it seems to be used to delete all config files when the last
konqueror exits... but this could be never (for people who never log out and with preloading enabled),
or too often (without preloading you close your only window, and the feature is useless since you can't
undo that).
How is it supposed to work, what should happen when a konqueror process quits? Can you still
restore windows from it? If no then it can delete its config file in all cases; if yes then it can't
delete its config file but we need a LRU list of config files (to delete the oldest ones) somewhere
on disk (in a global config file maybe?). This way we could restore windows even after quitting
the last konqueror instance (and even after logging out and back in....)
Please explain the design a bit (in code comments/docu, not in an email that we'll lose after that :)

If we do this list-of-config-files on disk, then we don't need all the fragile refcounting anymore, do we?
We'd just delete the N+1'th file when adding a new one...

About DBUS calls: don't use interface.call("methodname"), that's too fragile; use a generated dbus interface instead
(see any code that uses qt4_add_dbus_interface in a CMakeLists.txt file, like the favicon_interface.h generated in konqueror)
I also prefer the .xml file to be generated, but I know that not everyone agrees with that (at least this is rarely done...)

Some class docu for e.g. KonqClosedRemoteWindowItem wouldn't hurt and for KConfigNew (should be renamed to KonqClosedItemsConfig?).

There is some code duplication in KonqUndoManager... the "emit openClosedTab or emit openClosedWindow(remote) or emit openClosedWindow(local)" stuff is done twice, should be factorized.

Some cleanups are needed, too: kDebugs, no if(foo) before delete foo,
 no need to call locateLocal() if you give "appdata" as resource to KConfig anyway...
either pass the filename, or set the resource, but both is contradictory.

What's the reason for writing KConfigNew::self() by hand instead of the previous K_GLOBAL_STATIC(KConfigNew, s_config)?
Now you're leaking it, it's never deleted.

> I plan to add some test case after this and to fix a undo closed window 
> related bug and after that, I will consider the "undo closed window" feature 
> complete for now.

Unit tests would certainly be good :-)

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