A closer look at the recent files proposal



I finally got around to looking in more detail at the eggrecentchooser
stuff. 

My first comment is that 150+ functions and 10000+ lines of code
feel a bit large just for recent-files support. This is partially due to
the 3000 lines of XBEL parser, but also due to copying the file chooser
approach of having a EggRecentChooser interface with three
implementations (EggRecentChooserMenu for menus, EggRecentChooserDialog
for a standalone dialog, and EggRecentChooserWidget for an embedded
list) and a separate filter object. Are there any use cases for the
standalone dialog ? Who uses filters on recent files ?

I find it a bit confusing that there are two levels of filtering. In
particular I wonder if the filter function set on the manager affects
what items get written to disk ?

Why are the show-tips and show-numbers properties on the menu chooser
and not on the chooser interface ? Apart from the fact the we don't
exactly support tooltips on menus, don't they make sense in a dialog
as well ? Obviously, you would run into the problem that we don't
currently support tooltips on treeviews either, but I think it would
make sense to move these properties to the interface and just ignore
them if the implementation doesn't support them.

Apart from that, I just have a long list of small stylistic comments:

- Why is egg_recent_manager_changed() exported, the changed signal is
  not an action signal ?

- Naming, item vs info. The egg_recent_manager_functions are all named 
  _item (), but the object is called Info.

- Should use G_DEFINE_TYPE() to save on boilerplate.

- No point in using atomic refcounting for EggRecentInfo, since this 
  will become part of GTK+, so will go under the GDK lock.

- We should problably use the GLib stdio wrappers for stat() et al, even
  if Win32 will get a native implementation of recent files. 

- The previous point reminds me that someone needs to investigate this
  point: Can the recent manager api be implemented using the native
  Win32 API ?

- Just use if (!...) instead of if (FALSE == ...). 
  Similarly for == TRUE.

- Omit {} around if branches with single statements.


Can you commit your GMarkup XBEL parser to libegg ?


Regards, Matthias


 







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