Hi Sriram,

A good start on the slideshow music project, but a couple of changes are needed:

(i) User Interface Changes

In addition to all of the things that Adam mentioned above, I'd add
one more requirement: we need a volume slider, either in the Settings
dialog or possibly, when music playback is enabled, on the slideshow
tool palette itself. The reason I ask for this is two-fold. First, all
GNOME media applications provide an in-application volume control in
addition to the system volume control. For example, in Totem it
appears in the lower-right-hand corner of the main pane. Second, since
Shotwell slideshows run full-screen, access to the system volume
control on the GNOME panels isn't possible. So, in the present state
of affairs, you can't adjust the playback volume during a slideshow at
all.

I run Fedora, which by default doesn't come with an installed CODEC
for MP3 audio. So when I set my DEFAULT_SONG_URI to point to an MP3
file on my hard disk, I heard no audio played back and I got no error
message telling me why. GStreamer has facilities for dealing with this
through gstreamer-codec-install and gnome-codec-install. Take a look
at how they work. They should pop up a dialog box that explains the
name of missing codec and tries to use the system package manager to
find it.

(ii) Code Stuff

At AppWindow.vala:159, you've got the following lines:

    SlideshowPage? page = get_current_page() as SlideshowPage;
    if (page is SlideshowPage) {
        page.on_exit_slideshow();
    }

Note that the runtime type check (page is SlideshowPage) is
unnecessary, since the "as" operator returns a null reference if page
isn't a SlideshowPage. So your check on the next line (page is
SlideshowPage) really only needs to be (page != null). While it
doesn't matter so much here, you should know that GObject RTTI is
really, really, slow. So use it sparingly. If you can make one RTTI
call instead of two, all the better.

At SlideshowPage.vala:229 you've got two blank lines in a row. In
general, for Yorba projects, we use only one line of whitespace. Make
sure you read over the Yorba Coding Conventions here
http://trac.yorba.org/wiki/CodingConventions and ensure that any patch
you submit conforms to them.

This diff is very preliminary and is only a few lines long, so that's
about all I've got for now. If I can help you out in any way, with UI
specification, Vala language specifics, or whatever, feel free to drop
me a line.

Regards,
Lucas
_______________________________________________
Shotwell mailing list
[email protected]
http://lists.yorba.org/cgi-bin/mailman/listinfo/shotwell

Reply via email to