sön 2005-10-09 klockan 22:44 -0700 skrev Brad Taylor: > Hey, > > > I've been doing some hacking on Cowbell and now I've got a patch that > > fixes to many things at once, so now I'm going to try and split it up in > > smaller pieces. > > > > Attached is the biggest and uglies patch. It fixes some spacing, font > > size and similar issues in the glade files. > > Awesome! Thanks for your contributions. Heres the review: > > I agree completely with the changes you've made to the > SaveChangesDialog.glade. Good job. > > --- resources/PreferencesWindow.glade (revision 225) > +++ resources/PreferencesWindow.glade (arbetskopia) > @@ -17,6 +17,8 @@ > <property name="type_hint">GDK_WINDOW_TYPE_HINT_DIALOG</property> > <property name="gravity">GDK_GRAVITY_NORTH_WEST</property> > <property name="focus_on_map">True</property> > + <property name="urgency_hint">False</property> > + <signal name="delete_event" handler="OnDeleteEvent" > last_modification_time="Sun, 09 Oct 2005 09:31:44 GMT"/> > > <child> > <widget class="GtkVBox" id="vbox1"> > > Actually, any signals that were hooked up in the glade weren't meant to > be there; we actually hook these up in code. > Ok. I was actually planing on auto connecting these so we don't have to do it in the code :-) > @@ -28,51 +30,26 @@ > <widget class="GtkVBox" id="vbox2"> > <property name="visible">True</property> > <property name="homogeneous">False</property> > - <property name="spacing">6</property> > + <property name="spacing">16</property> > > Good catch, however as specified in the Gnome HIG > (http://developer.gnome.org/projects/gup/hig/2.0/design-window.html), > this should be 18. > Yes of course. Just a typo. > @@ -278,21 +232,35 @@ > </child> > > <child> > - <widget class="GtkCheckButton" id="CoverImageCheckbox"> > + <widget class="GtkAlignment" id="alignment3"> > <property name="visible">True</property> > - <property name="can_focus">True</property> > - <property name="label" translatable="yes">Cache album > cover along with songs</property> > [snip] > + <property name="label" translatable="yes">_Save > album cover along with songs</property> > > > Actually, we explicity use "Cache" to hopefully get around some of > Amazon's legal policies. They only want us "caching" our results for a > certian time, so this language change doesn't really do anything, but at > least it makes us seem like we're doing the right thing. > > > Index: resources/AboutDialog.glade > =================================================================== > --- resources/AboutDialog.glade (revision 225) > +++ resources/AboutDialog.glade (arbetskopia) > @@ -4,12 +4,12 @@ > <glade-interface> > > <widget class="GtkDialog" id="window"> > - <property name="border_width">12</property> > + <property name="border_width">6</property> > > I kind of like this at 12... it keeps it consistent with the > PreferencesWindow. > You forget that the action area in a GtkDialog has a border too. The dialog should actually have a border of 7 px, since the border of the action area is 5 px, and not 6 px as I first thought. > > @@ -32,7 +33,7 @@ > <property name="layout_style">GTK_BUTTONBOX_END</property> > > <child> > - <widget class="GtkButton" id="closebutton1"> > + <widget class="GtkButton" id="CloseButton"> > > Since we don't reference it in code, this change doesn't need to be > made. > > > Index: resources/MainWindow.glade > =================================================================== > --- resources/MainWindow.glade (revision 225) > +++ resources/MainWindow.glade (arbetskopia) > @@ -8,8 +8,8 @@ > <property name="type">GTK_WINDOW_TOPLEVEL</property> > <property name="window_position">GTK_WIN_POS_NONE</property> > <property name="modal">False</property> > - <property name="default_width">700</property> > - <property name="default_height">500</property> > + <property name="default_width">600</property> > + <property name="default_height">375</property> > > I don't agree with this change. When importing more than 11 songs (a > typical album has 12 songs), the window becomes too small to hold > everything easily. > Sorry, I forgot about different font sizes. When using a font size of 8, it feels to big and obviously it's too small using larger fonts, so I changed the default size to 700x450, which seems to be a good compromise. > Also, I don't really agree with the spacing changes either. It seems > like you're trying to get Cowbell to run on a 640x480 monitor -- is this > true? Most applications use a horizontal spacing of 12 (as specified by the HIG) and a vertical spacing of 6. Since I have no strong feelings about this I changed it back to 12. I have also added mnemonics to the labels in the main window. > > Index: resources/ProgressWindow.glade > =================================================================== > --- resources/ProgressWindow.glade (revision 225) > +++ resources/ProgressWindow.glade (arbetskopia) > @@ -4,7 +4,7 @@ > <glade-interface> > > <widget class="GtkDialog" id="window"> > - <property name="border_width">12</property> > + <property name="border_width">6</property> > > According to the HIG, this should be 12. Same as above... > > Everything else looks alright, from what I can tell. If you make the > changes that I mentioned, remove all the signals that are in the glade > and add a ChangeLog entry, I'll look into commiting it. Overall, you've > done very good work! I look forward to seeing more patches from you in > the future. > > One note, however -- would you mind sending future patches to > cowbell-list lists gnome org? This way, it'll be easier for me to keep > track of what you've sent. > > Thanks! > > -Brad
Attachment:
cowbell-ui.patch.gz
Description: GNU Zip compressed data