After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723566 - Use of listbox widget for document panel
Use of listbox widget for document panel
Status: RESOLVED FIXED
Product: gedit
Classification: Applications
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Gedit maintainers
Gedit maintainers
Depends on:
Blocks:
 
 
Reported: 2014-02-03 22:13 UTC by sébastien lafargue
Modified: 2014-02-08 19:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use of listbox widget for document panel (86.50 KB, patch)
2014-02-03 22:14 UTC, sébastien lafargue
none Details | Review
Use of listbox widget for document panel ( rebase ) (86.49 KB, patch)
2014-02-04 06:59 UTC, sébastien lafargue
none Details | Review
rewrite of document panel (68.36 KB, patch)
2014-02-05 19:01 UTC, sébastien lafargue
none Details | Review
Use of listbox widget for document panel (66.58 KB, patch)
2014-02-06 17:21 UTC, sébastien lafargue
none Details | Review
Use of listbox widget for document panel (66.28 KB, patch)
2014-02-06 18:39 UTC, sébastien lafargue
none Details | Review
Use of listbox widget for document panel (65.30 KB, patch)
2014-02-07 12:46 UTC, sébastien lafargue
none Details | Review
Use of listbox widget for document panel (72.27 KB, patch)
2014-02-07 18:17 UTC, sébastien lafargue
none Details | Review
Use of listbox widget for document panel (72.26 KB, patch)
2014-02-07 21:45 UTC, sébastien lafargue
none Details | Review
Use of listbox widget for document panel (71.52 KB, patch)
2014-02-07 22:41 UTC, Paolo Borelli
none Details | Review
Use of listbox widget for document panel (71.93 KB, patch)
2014-02-08 17:08 UTC, sébastien lafargue
committed Details | Review

Description sébastien lafargue 2014-02-03 22:13:06 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]
Comment 1 sébastien lafargue 2014-02-03 22:14:27 UTC
Created attachment 268011 [details] [review]
Use of listbox widget for document panel
Comment 2 sébastien lafargue 2014-02-04 06:59:22 UTC
Created attachment 268030 [details] [review]
Use of listbox widget for document panel ( rebase )
Comment 3 Paolo Borelli 2014-02-04 08:23:16 UTC
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
Comment 4 sébastien lafargue 2014-02-05 19:01:21 UTC
Created attachment 268198 [details] [review]
rewrite of document panel
Comment 5 Paolo Borelli 2014-02-05 20:15:21 UTC
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
Comment 6 sébastien lafargue 2014-02-06 17:21:51 UTC
Created attachment 268313 [details] [review]
Use of listbox widget for document panel
Comment 7 sébastien lafargue 2014-02-06 18:39:55 UTC
Created attachment 268328 [details] [review]
Use of listbox widget for document panel
Comment 8 sébastien lafargue 2014-02-07 12:46:50 UTC
Created attachment 268401 [details] [review]
Use of listbox widget for document panel
Comment 9 sébastien lafargue 2014-02-07 18:17:37 UTC
Created attachment 268438 [details] [review]
Use of listbox widget for document panel
Comment 10 sébastien lafargue 2014-02-07 21:45:14 UTC
Created attachment 268454 [details] [review]
Use of listbox widget for document panel
Comment 11 Paolo Borelli 2014-02-07 22:41:42 UTC
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.
Comment 12 sébastien lafargue 2014-02-08 17:08:14 UTC
Created attachment 268510 [details] [review]
Use of listbox widget for document panel