Hi Alberts, hi All, On 2013-09-27 14:54, Alberts Muktupāvels wrote:
[... private part snipped ...] Philipp Kaluza wrote:The gnome-3-8 branch is aimed at at the 3.8.1 etc. releases, master currently at 3.10.Yes, currently I test in 3.8. I will use that gnome version which next ubuntu will use. I guess ubuntu 14.04 will use 3.10. Isn't easier to work currently with only master branch? Working with master and gnome-3-8 branch will require all changes apply twice? Or am I wrong?
Yes, all changes appropriate to both branches need to be applied twice. But that's the point - we also want to keep supporting dowstreams that pick this up for a 3.8-based desktop, e.g. current unstable or possibly saucy. 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.
I'd give commit access to you, but still expect us to have a review process in place. In the gnome world we unfortunately don't have a gerrit installation, so the review tool of choice is attaching patches to bugzilla bugs, then pestering people on the mailing list to review them. ^^So. I still will have to create patch, create bug report and ask to review patch before commiting?
For trivial patches, just commit them. For non-trivial patches, yes, please let's review them in bugzilla. Though you don't need to ping me on-list for every patch - I will get notified about new patches appearing in bugzilla. If I don't manage to review something within two weeks or so, please ping me. #################################### # The Code review part #################################### This time, I'll do the review on-list, for people to follow / chip in. On 2013-09-27 22:51, Alberts Muktupāvels wrote:
Hi! Attached new patches for gnome-panel... 0001 - Changed deprecated gtk_[hbox/vbox]_new to gtk_box_new. 0002 and 0003 - Replaced deprecated GtkTable with GtkGrid.
First of all, getting rid of deprecated widgets is a very worthy goal - thank you for working on this. As it has the potential for regressions, as we saw with commit dde999b0ba9d444f5d97ff13179a585dfe1ee711, we should aim these only at master.
From your patch 0003, in function panel_widget_new():
if (packed)
panel->size = 0;
else
- panel->size = G_MAXINT;
+ panel->size = 0;
+ /* Why panel size was set to G_MAXINT? Replacing GtkTable with
GtkGrid this was causing problems! */
In such a case, please try to investigate what the reason was for
setting it that way. If you are unsure, please ask for help on the list.
A comment is _not_ a good place to document "I have no idea what I'm
doing here" !
Now I have replaced GtkTable with GtkGrid int gnome-panel/panel-toplevel.c too (0003 patch), but it required small change in gnome-panel/panel-widget.c. Do you know why creating new panel widget its inital size (par horizontal panel - width) was set to G_MAXINT? This caused problems for gtkgrid. I got warnings "Gdk-WARNING **: Native children wider or taller than 65535 pixels are not supported"
I think this might be a legacy from the old layouting code. We might want to override https://developer.gnome.org/gtk3/stable/GtkWidget.html#gtk-widget-set-allocation to make sure the widgets always use the full screen width / height (only for expanded panels ofcourse). 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 ? ###############
0004 - Fixed regression introduced by me. Moving applet to different position does not work correctly. gtk_get_current_event_device was not best way to get device... In my case it often returned NULL when tried to move applet.
What exactly you want discuss about 0004 patch? You applied one of my patches (https://git.gnome.org/browse/gnome-panel/commit/?id=dde999b0ba9d444f5d97ff13179a585dfe1ee711). Seems that it introduced regression. I added extra applet to panel and tried it moving. It was almost impossible to move to correct place.
Yes, I can confirm the regression, and we should definitely fix this in the 3.8 branch also.
I was getting many critical errors in terminal about invalid GTK_DEVICE, because that function was returning NULL. So it is not safe to use at least that way I was using it. Googled for other solution to replace gtk_widget_get_pointer. Solution in patch now works ok for me - I can move applet to panel start, middle or end without problems. I can move applet to different panel too. So now it realy works. Before this fix, I tried one other way, but that didn't work good, I was not able to move applet to different panel.
OK, but please also discuss what you did here. *For review purposes only*, I reverted commit dde999b0ba9d444f5d97ff13179a585dfe1ee711 locally, then generated a diff to your new solution (attached). So this would be an updated version of your original "fix deprecated warnings (part 1)" patch, as it would have been created if we had caught this regression during code review. This allows us (me) to better see where we were coming from. So you tried to replace the deprecated gtk_widget_get_pointer (some_widget, ...); first with gdk_window_get_device_position(gtk_widget_get_window(some_widget),gtk_get_current_event_device(),...); but that caused problems because gtk_get_current_event_device () will return NULL outside of event handlers. Your new solution is device=gdk_device_manager_get_client_pointer(gdk_display_get_device_manager(gtk_widget_get_display(some_widget))); gdk_window_get_device_position(gtk_widget_get_window(some_widget),device,...); 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 ? 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. "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. 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.
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.
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 see we have more to investigate here.
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.
Currently there is many indentation problems. Personally I dislike braking line in more lines to not exceed 80 chars in line. I would love to take time and reformat code. That why more or less I would become more familiar with code. How things work and so on. What do you think about this?
No, we cannot do that at the moment (not even on a single branch), as it would make it impossible to re-merge with mate-panel in any sane way. But feel free to familiarize yourself with the code as much as you want. :)
Sorry about my English...
No need to apologize, for most people on the internet English is their secondary language. Thank you so much for your hard work, keep at it ! :-) Cheers Philipp -- Philipp Kaluza Ghostroute IT Consulting
Attachment:
UPDATED-panel-fix-deprecated-warnings-part-1.patch
Description: Text Data