GNOME Bugzilla – Bug 723566
Use of listbox widget for document panel
Last modified: 2014-02-08 19:00:54 UTC
It's an almost complete rewrite of document panel : the row part is now a row widget in file gedit-documents-panel-row.[c|h]
Created attachment 268011 [details] [review] Use of listbox widget for document panel
Created attachment 268030 [details] [review] Use of listbox widget for document panel ( rebase )
Review of attachment 268030 [details] [review]: Just a couple of quick comments, I'll try to look in more detail later ::: gedit/gedit-debug.c @@ -105,2 @@ if (g_getenv ("GEDIT_DEBUG_PANEL") != NULL) debug = debug | GEDIT_DEBUG_PANEL; No need to add an ad hoc debug category for this: these categories are generic areas of gedit codebase, not 1:1 relation with classes and objects. Just use DEBUG_PANEL ::: gedit/gedit-documents-panel-row.c @@ +1,1 @@ +/* I wonder if it would be cleaner to have DocumentRow and GroupRow instead of a sinfgle row with a type. I also suggest to just keep the row object private to the panel, that is keep the implementation inside docs-panel.cm, this should allow to cut down the boilerplate a bit
Created attachment 268198 [details] [review] rewrite of document panel
Review of attachment 268198 [details] [review]: I have no looked at the actual C code yet... here are some more prelim comments ::: gedit/gedit-documents-panel.c @@ +18,3 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA I think here you mis-merged a conflict (similar issue in the .h) ::: gedit/gedit-documents-panel.h @@ -71,2 +79,4 @@ GtkWidget *gedit_documents_panel_new (GeditWindow *window); +typedef struct _GeditGenericRowPrivate GeditGenericRowPrivate; + What I meant is that all the row objects should go in the C file, including the the macros etc so that they are not visibile at all from outside (equivalent to "inner classes" in java or c#) That way you do not need to split out a "priv" for every class and some more boilerplate can be removed. Lastly, the names need to be more specific (GeditDocumentsPanelRow, GeditDocumentsPanelGroupRow, GeditDocumentsPanelTabRow) since the gobject type names are in a global namespace @@ +92,3 @@ +typedef struct _GeditGroupRowPrivate GeditGroupRowPrivate; + +#define GEDIT_IS_GROUP_ROW(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GEDIT_TYPE_GROUP_ROW)) Feel free to remove these comments... I know gedit files have them because they come from the same template, but they are rather obvious and we have enough boilerplate as is :P ::: gedit/gedit-style.css @@ +33,3 @@ } + +.gedit-document-panel { The css should probably go in adwaita: in the local css we just put stuff like removing borders etc that are common to all the themes, but colors etc should go in the theme itself
Created attachment 268313 [details] [review] Use of listbox widget for document panel
Created attachment 268328 [details] [review] Use of listbox widget for document panel
Created attachment 268401 [details] [review] Use of listbox widget for document panel
Created attachment 268438 [details] [review] Use of listbox widget for document panel
Created attachment 268454 [details] [review] Use of listbox widget for document panel
Created attachment 268464 [details] [review] Use of listbox widget for document panel I amended a few minor things and I was almost pushing this when I found a couple of bugs: - when you hover the mouse on the first row it starts to flicker - when you have a file with a long name and you hover the mouse the X button is not shown because there is no space allocated to it. Also note that I removed the CSS changes for now, since they should go in adwaita (at least for now) Please restart from this patch since I have fixed a few minor things.
Created attachment 268510 [details] [review] Use of listbox widget for document panel