Re: [gnome-flashback] On branches, code review and commit guidelines (was: New patches for gnome-panel)

[Alberts Muktupāvels <alberts muktupavels gmail com>]

Hi All!

> It will be good if you can keep a 3.8 gnome environment on your hard
> disk for testing purposes, e.g. a copy of your current Ubuntu installation.
Ok.

> On replacing GtkTable with GtkGrid:
> When we look at what the table is actually used for, it is a 3x3 grid
> with buttons on top, bottom, left, right to fold the panel in, but only
> two buttons are visible at any time. Nowadays we have GtkBox, whose
> orientation we can change on-the-fly when the panel is realigned
> horizontally / vertically. What do you think about replacing the Table
> with a GtkBox here ? I think it will make the codebase simpler in the
> medium term.
>
> In either case,
> -       GtkWidget              *table;
> +       GtkWidget              *grid;
> I would like so see a generic name here (so we don't have to rename
> again), and as this is a private data structure, we could use the
> correct type, this might avoid some type casting. E.g.
> +       GtkBox              *outer_container;
>
> What do you think ?
We want use GtkGrid anyway
(https://developer.gnome.org/gtk3/3.7/gtk-migrating-GtkGrid.html).
Currently simplier is to do what I did. Otherwise we will need add
more changes, but currently I don't want do it. I mean changing from
3x3 grid to single line will require changes how left, right, up, down
arrows are added to panel. Do we really need it now? I guess for now
just change to GtkGrid. We can latter make more changes to simplify
code. I will write other e-mail about some other problems I have
found.

> Reading through the documentation, your solution seems sensible for now.
> Medium-term, however, it might make sense to cache the gdk_device
> per-panel. What do you think ?
Probably, but at least for now I will not do it. Maybe latter. I am
already spending too much time on gnome-panel...  I have much work to
do in my paid job.

> The following issues remain with patch 0004:
> - Please prefix your commit message with the affected components, in
> this case "panel, libpanel-applet:"
> - Write a better commit message, not just "... was not the best way".
> Something similar to my analysis above might be appropriate.
> - Whitespace issues: Sometimes you replace a line, that begins with a
> TAB character, by one beginning with four spaces. I think the codebase
> assumes 8-space-tabs, but the important thing is to keep the tab in place.
> - Whitespace issues (2): Sometimes you also add a blank line, that
> contains spaces or tabs. Blank lines should be 0 characters long.
Ok. My fault, for some reason i have checked in gedit to add spaces
instead of tab. I guess it does not matter tab width I set in gedit,
right? I prefer to use 4 space tab.

> "git am" even warned me about the latter, and I only saw the former by
> chance when viewing the patch in gedit.
>
> If you use vim, I can send you a configuration snippet that will
> highlight some of these errors.
No, I am using gedit.

> In the absence of a proper Coding Style guideline, it is important to
> always emulate the style of the surrounding code, and clean up only the
> parts of the code you touch anyhow.
Ok.

> > I will continue to work with gnome-panel as I want it to work with
> > newest ubuntu versions. I dont like unity, gnome-shell or similar
> > things. I prefer only gnome-panel.
> >
> > I am using separate x screens (nvidia), but gtk 3.10 has removed
> > support for multiple screens.
> Could you send me a pointer to where this is documented ? I was unaware
> of this until now.
Found this one -
https://mail.gnome.org/archives/commits-list/2013-April/msg00571.html

> >  That means there will be deprecated
> > warnings about screens too. But please don't accept any patches that
> > removes multi screen support from gnome-panel and/or metacity. Someone
> > told that we should be able to use other screens by manually opening
> > them as displays. I have idea which I will try to realize and if
> > current code will be removed it will make it harder to me. I know that
> > separate x screens now days are rarely used. I hope you and other devs
> > will not want to remove support for such setup.
> Ok, if I get this correctly, your nvdidia driver exposes seperate
> monitors as display :0.0, :0.1 etc, right ? And that is deprecated ?
I am using /etc/X11/xorg.conf to set up separate X screens. Yes,
currently I have 2 displays (multiseat). First with 3 screens, each
screen one monitor (:0.0, :0.1, :0.2); Second with 1 screen, 1
monitor. If I understand correctly than my current setup will be
useless. Only default screen will work, other screens will be black??
I guess I still will be able to move cursor to other two screens. My
idea is to get all available displays and default one. (I guess gtk
3.10 will return 3 displays or 4 displays if I wont use multiseat).
Than create available displays list which will contain only displays
which name in my case starts with :0. Than using this displays list
instead screen list.

I think it should work, I have not tested it. If I remember correctly
than if I switch from separate x screens to tvinview than gdk will
return all monitors as on display, screen. So there will be only one
display :0.0 or :1.0. So it should be safe to use?

Can someone confirm my idea? Mostly I want know If displays names
:0.0, :0.1, :0.2 are created only when cunfigurated as separate
screens? When I tried on my laptop with external monitor I was only
getting one display name :0.0 even I had two monitors.

First number - display, second number - screen? So :2.3 would mean
third screen on second display?

> I see we have more to investigate here.
I will try to do it, bet only when ubuntu will switch to gtk 3.10. Gtk
3.10 will break all applications supporting multiple screens, beacause
gdk_display_get_n_screens will always return 1.
(https://developer.gnome.org/gdk3/stable/GdkDisplay.html#gdk-display-get-n-screens)

> > One more thing. Shouldn't decide code formatting rulles to follow
> > writing code?
>
> It would be nice to have a formal document for this, but in its absence,
> please stick to the style of the surrounding code, and see what I wrote
> above.
Ok.

--
Alberts Muktupāvels