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.


* eggdesktopbookmarks.[hc]:

 - Where is the GMarkup parser ? libegg still seems to have libxml

 - 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 ?!
 - 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.

 - 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 ?

* eggrecentchooser.[hc], eggrecentchooserprivate.h

 - I wonder why there is both EggRecentChooserSortType and
   EggRecentSortType, and why they are not the same.

 - 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 ? Also the documentation for these
   is a bit weak. 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.
   Also, it seems that most of these properties are only implemented
   for some of the EggRecentChooser implementations.

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

* eggrecentmanager.[hc]


   Should these really be treated as hard error conditions ? 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 ?

* Random coding style comments

 - Please don't use boolean == TRUE
 - Please use G_DEFINE_TYPE where appropriate

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]