Created attachment 107837 [details] [review]
Patch removing use of deprecated libgnomeui and deprecated icons

Patch also makes the icons themeable

Created attachment 107838 [details]
icons

Untar this in the gst-mixer directory and add the files to SVN

Committing this patch will also solve bug 310883

rock! I'll review the patch ASAP

I again, I am missing the XML files! Thanks

Created attachment 107855 [details] [review]
Use GtkUIManager

Crap, I was working on this bug at the same time, due to the gnome-love keyword.
Lot of the code is similar to Jaap's, coincidentally.

(In reply to comment #6)
> Created an attachment (id=107855) [edit]
> Use GtkUIManager
> 
> Crap, I was working on this bug at the same time, due to the gnome-love
> keyword.
> Lot of the code is similar to Jaap's, coincidentally.
> 

oooh, guys...! it's a shame that you did not say that you were working on this, both of you.. given that those two patches are significantly big! It makes my maintainer work more difficult, what I am suppose to do..?

Craig, can you point out what is missing in Jaap code?
Jaap, can you point out what is missing in Craig code?

It would be much better to do several patches, ex:

1a. G_DEFINE_TYPEs..
1b. add missing dependencies in configure
2a. GnomeApp->GtkWindow
2b. ->GtkStatusBar
3. icons
4. rewrite callbacks to get rid of casts
5. improve about dialog
7. UIInfo->UIManager
8. add missing license in header
...

Using a distributed version control make that easy, and we could pick/compare patch easily.

Or please, found a solution to this dilemna :)

Thanks to both of you anyway, it's all for the best!

I wasn't sure if I would finish the patch, so I didn't want to announce I was working on it. It just happened I did finish it.

Jaap's has improved the Icon code and About box code, I didn't look at that. Jaap's uses a menu place holder "/MainMenu/File/FileChangeDevice/Devices Placeholder" , which shouldn't be necessary.

In Jaap's patch, the line:
gconf_client_get_int (win->client,PREF_UI_WINDOW_HEIGHT, NULL);
should be:
height = gconf_client_get_int (win->client,PREF_UI_WINDOW_HEIGHT, NULL);
(this was a bug in the old code too)

My code makes gnome_volume_control_window a little more 'GObject' like.

Created attachment 107862 [details] [review]
Updated patch containing missing xml file and the height gconf fix

Oops forgot to include the xml file.

Indeed a pain that we don't have distributed version control system, and that by accident two people are working on the same thing.

Breaking the patches up will generate quite some extra work. Can I suggest we apply my patch (because it's the largest). After that we make a patch on that code with the improvements of Craig?

What do you think about that?

Jaap, Craig, thanks for discussing the merge.

(In reply to comment #9)
> Breaking the patches up will generate quite some extra work. Can I suggest we
> apply my patch (because it's the largest). After that we make a patch on that
> code with the improvements of Craig?

Ok for me. What about you Craig?

Just one comment about the xml file, wouldn't it be more clever to have it inlined? (less files, less changes...)? 

(In reply to comment #10)
> 
> Just one comment about the xml file, wouldn't it be more clever to have it
> inlined? (less files, less changes...)? 
> 
Most GNOME apps use the xml file, but I can inline it if you prefer. Let me know

Craig if you also agree I have SVN access, so I can make the changes

(In reply to comment #11)
> Most GNOME apps use the xml file, but I can inline it if you prefer. Let me
> know

fair enough.

> Ok for me. What about you Craig?

Agree, review/commit Jaap's patch - it is better and has more features. I'll suggest more improvements if I see them.

Jaap, go ahead.

2008-03-24  Jaap Haitsma  <jaap@haitsma.org>

	reviewed by: Marc-Andre Lureau

	* configure.in:
	* gst-mixer/Makefile.am:
	* gst-mixer/gnome-volume-control-ui.xml:
	* gst-mixer/icons/16x16/Makefile.am:
	* gst-mixer/icons/16x16/devices/Makefile.am:
	* gst-mixer/icons/16x16/status/Makefile.am:
	* gst-mixer/icons/Makefile.am:
	* gst-mixer/pixmaps/Makefile.am:
	* gst-mixer/src/Makefile.am:
	* gst-mixer/src/button.c: (gnome_volume_control_button_class_init),
	(gnome_volume_control_button_init),
	(gnome_volume_control_button_dispose),
	(gnome_volume_control_button_new),
	(gnome_volume_control_button_clicked),
	(gnome_volume_control_button_mouseover),
	(gnome_volume_control_button_mouseout):
	* gst-mixer/src/button.h:
	* gst-mixer/src/element.c:
	(gnome_volume_control_element_class_init),
	(gnome_volume_control_element_init),
	(gnome_volume_control_element_new),
	(gnome_volume_control_element_dispose),
	(gnome_volume_control_element_change):
	* gst-mixer/src/element.h:
	* gst-mixer/src/main.c: (main):
	* gst-mixer/src/misc.c:
	* gst-mixer/src/preferences.c:
	* gst-mixer/src/stock.h:
	* gst-mixer/src/track.c: (gnome_volume_control_track_add_title),
	(gnome_volume_control_track_put_switch),
	(gnome_volume_control_track_add_playback),
	(gnome_volume_control_track_add_recording),
	(gnome_volume_control_track_add_switch),
	(gnome_volume_control_track_add_option):
	* gst-mixer/src/track.h:
	* gst-mixer/src/volume.c: (gnome_volume_control_volume_class_init),
	(gnome_volume_control_volume_init), (cb_mouseover), (cb_mouseout),
	(get_button), (gnome_volume_control_volume_new),
	(gnome_volume_control_volume_dispose),
	(gnome_volume_control_volume_size_alloc),
	(gnome_volume_control_volume_expose):
	* gst-mixer/src/volume.h:
	* gst-mixer/src/window.c: (menu_item_select_cb),
	(menu_item_deselect_cb), (connect_proxy_cb), (disconnect_proxy_cb),
	(cb_change), (cb_exit), (cb_preferences_destroy), (cb_preferences),
	(open_uri), (cb_help), (cb_about),
	(gnome_volume_control_window_dispose),
	(gnome_volume_control_window_class_init),
	(gnome_volume_control_window_init),
	(gnome_volume_control_window_new):
	* gst-mixer/src/window.h:
	Use GtkUIManager i.s.o. deprecated libgnomeui
	Remove libgnomeui as a dependency
	Use tango icons if possible and make all icons themeable
	Install deprecated icons from the application itself. Now we just need
	a pixel pusher to make us some nice tango icons for the remaining icons
	Use G_DEFINE_TYPE macro for our own types
	Fixes bug #494031 and bug #310883

Commited

Let's attach the improvements of Craig here as well

Created attachment 107916 [details] [review]
Patch removing prototypes of static functions and uses G_DEFINE_TYPE for preferences as well

Some other cleanups:
1) Reshuffling of functions such that prototypes are not necessary anymore for static functions
2) Use G_DEFINE_TYPE for preferences
3) Some other small cleanups

Can I apply?

(In reply to comment #16)
> Created an attachment (id=107916) [edit]
> Patch removing prototypes of static functions and uses G_DEFINE_TYPE for
> preferences as well
> 
> Some other cleanups:
> 1) Reshuffling of functions such that prototypes are not necessary anymore for
> static functions
> 2) Use G_DEFINE_TYPE for preferences
> 3) Some other small cleanups
> 
> Can I apply?
> 

Please avoid reshuffling, that makes "blame" difficult to read :) Furthermore, prototypes are ok (even necessary in some C standard)

Quite some GNOME programs use this no prototype for static functions. Furthermore programs that do it have it usually not for all functions because people forget about it when they add a new function.

I don't see the problem with reshuffling and blame so much. In the end it's not about blaming but about fixing ;-) And I'll be happy to help fixing.

(In reply to comment #18)
> Quite some GNOME programs use this no prototype for static functions.
> Furthermore programs that do it have it usually not for all functions because
> people forget about it when they add a new function.
> 
> I don't see the problem with reshuffling and blame so much. In the end it's not
> about blaming but about fixing ;-) And I'll be happy to help fixing.
> 

Prototypes are not necessary, right. But please avoid reshuffling. Only when necessary, and here, I don't think we should. Reshuffling does not fix anything.

OK, Can I commit the G_DEFINE_TYPE part in the preferences? This way that will be consistent with the rest again

(In reply to comment #20)
> OK, Can I commit the G_DEFINE_TYPE part in the preferences? This way that will
> be consistent with the rest again
> 

Can you attach the patch please?

Created attachment 107925 [details] [review]
The G_DEFINE_TYPE part

(In reply to comment #22)
> Created an attachment (id=107925) [edit]
> The G_DEFINE_TYPE part
> 

looks ok to me! thanks for the update, please apply

Commited