Re: libegg/recentchooser

[Emmanuele Bassi <ebassi gmail com>]

Hi Matthias,

On Tue, 2005-11-29 at 12:18 -0500, Matthias Clasen wrote:
> Hey Emanuele,
> 
> in an attempt to get this moving forward again, I spent some time
> looking at the code thats currently in libegg, and wrote down some
> random notes.

Thanks.  I'll try and do my best to answer.

> 
> Matthias
> 
> 
> * eggdesktopbookmarks.[hc]:
> 
>  - Where is the GMarkup parser ? libegg still seems to have libxml

It's in libegg/bookmarkfile - I didn't remove the old libxml-based
parser for reference checking and because CVS removal always scare the
hell out of me. :-)

>  - Should the name be egg_desktop_bookmarks_... ? Considering it does
>    not contain a single bookmark, but a whole list of resources. But 
>    then maybe it should not be named "bookmark" at all, since this is 
>    about recently used resources ?!

The name should be BookmarkFile/bookmark_file since it retains the same
semantics as the KeyFile/key_file object.  Also, the BookmarkFile code
should not be limited to recently used resources: the shortcuts for the
FileChooser could use it - both internally and as a public API for
manipulating the shortcuts.  Right now, the only way to access this data
is to copy the code from the FileSystem object or to replicate it (see
bug #147434 [1]); also, the same meta-data about the application could
make the shortcuts application-specific (see bug #157377 [2]).

>  - Is there any particular reason why all the load functions return the
>    item count as well, when there is egg_desktop_bookmark_get_count() ?
>    Might be nice to avoid that out parameter.

The BookmarkFile code has this function - it was a later addition.

>  - Is EGG_DESKTOP_BOOKMARK_ERROR_DUMP just a catch-all for bugs which 
>    have no other code ? If so, you may want to move it to the first 
>    position. Or is it a generic code for all error having to do with 
>    writing/modifying bookmarks ? No, that would be _WRITE...

>  - I'm not convinced that having GErrors in all the getters is adding 
>    a lot of value. In most cases, a NULL return value already tells 
>    the whole story, no ?

I was trying to "mimic" the KeyFile API.  We have non-mandatory
meta-data, though; returning %NULL could either mean that the attribute
was unset *or* the item was missing.  I prefer to keep the former case
possible, while the latter an error.

> * eggrecentchooser.[hc], eggrecentchooserprivate.h
> 
>  - I wonder why there is both EggRecentChooserSortType and
>    EggRecentSortType, and why they are not the same.

Uhm, never quite convinced me either - but in design phase I preferred
to keep them separated, as the first affects the RecentManager, while
the second affects the RecentChooser implementations.

I could merge them, though, and place the SortType into a common
enum-only header.

>  - Why do we need to add so many "UI tweak" properties ? 
>    I can see that show-private, show-not-found and local-only 
>    make sense, but what is the rationale for show-tips,
>    show-numbers and show-icons ?

The "show-tips" and "show-numbers" properties were left-overs from the
adaptation of the EggRecent API; "show-tips" might just go, while it was
pointed out (on IRC) that a mnemonic for the menu would be in order, in
order to navigate with keyboards.  Also, the "show-icon" could be
usefule: while menus have a style property, the RecentChooserWidget
should listen to that property, and that would violate a bit the
separation between the menu and the widget.

>  Also the documentation for these
>    is a bit weak.

I know.  Not being a non-english speaker makes writing good
documentation more difficult, for me at least.

>  It does not mention that e.g. the visibility
>    of icons in menus still depends on global settings. I also
>    wonder how well the numbering code works in RTL locales.

For this, I'll need more testing.  I'll see what I can do.

>    Also, it seems that most of these properties are only implemented
>    for some of the EggRecentChooser implementations.

Some of them make more sense in some implementations, e.g. the
"show-tips" would be useless on the RecentChooserWidget without the
TreeView supporting them.  I was planning to add tips to the filters,
though, and those would be controlled by the "show-tips" property.
Other properties are partially implemented because I was still
considering to have them.  As I said, the "show-numbers" property might
go away: we could just number the items ourselves (even though I don't
really much like it); the "show-tips" property is useful only if every
widget supports tooltips, which should happen when the RecentChooser
code hits GTK+.

>  - The item-activated signal is in eggrecentchooserprivate.h, yet
>    it seems to be required api to make menus work, like in 
>    test-recent-menu.c

You're right - I'll export the RecentChooser interface.

> * eggrecentmanager.[hc]
> 
>  - EGG_RECENT_MANAGER_ERROR_NOT_REGISTERED
>    EGG_RECENT_MANAGER_ERROR_BAD_EXEC_STRING
> 
>    Should these really be treated as hard error conditions ?

I prefer being a little more generous on GErrors than not.  Anyway,
these could go away.

>  I think 
>    it would make sense to interpret data->app_name == NULL as use 
>    g_get_application_name(), and I think not setting a command line 
>    should be fine. Why does every file have to have an associated 
>    commandline ?

Because we would not know how to launch the URI using that application.
Sometimes "app-name <uri>" could be enough, but some application might
use command-line switches, or they do not support URIs at all.  A
command-line is needed to let developers decide how their application
should open a recently used resource, without actually having to
implement a launch API by ourselves, replicating g_spawn_* functions.
The developer will just use the invocation method he or she sees fit.

> * Random coding style comments
> 
>  - Please don't use boolean == TRUE

Will do.

>  - Please use G_DEFINE_TYPE where appropriate

Ditto.

> When running test-recent-menu I noticed that the widget makes
> not-found items insensitive, while the menu does not.

It was done to test both cases. :-)

+++

Thanks for your review.

Ciao,
 Emmanuele.

+++

[1] http://bugzilla.gnome.org/show_bug.cgi?id=147434
[2] http://bugzilla.gnome.org/show_bug.cgi?id=157377

-- 
Emmanuele Bassi - <ebassi gmail com>
Log: http://log.emmanuelebassi.net