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 - <[EMAIL PROTECTED]> Log: http://log.emmanuelebassi.net _______________________________________________ gtk-devel-list mailing list gtk-devel-list@gnome.org http://mail.gnome.org/mailman/listinfo/gtk-devel-list