Merge lp:~davidgomes/noise/code-style-fix into lp:~elementary-apps/noise/trunk

[David Gomes]

Fixed lots of awful code.

[lp:~davidgomes/noise/code-style-fix]

1063. By David Gomes on 2012-10-06

More fixes.

[David Gomes (davidgomes)]

Also, I think all that commented code in the end should be removed, it just looks bad.

[Rico Tzschichholz (ricotz)]

might be better to use a namespace like

namespace Noise {

    public class GStreamerTagger : GLib.Object {

    ...

    }

}

[Victor Martinez (victored)]

Thanks for your work David.

I'm not a big fan of Noise's code, and I don't particularly like how all these classes (LibraryManager.vala, FileOperator.vala, GStreamerTagger.vala) were put together in the first place, and because of that I started a rewrite some months ago but put in on hold as it would be impossible to finish it on time for Luna: http://bazaar.launchpad.net/~victored/noise/core-refactor/files/head:/core/GStreamer/Metadata/

Essentially, the problem here is that Noise lacks modularity and doesn't make full use of polymorphism and data hiding (among other problems, like code duplication, etc.) Modifying a single public variable in one class leads you to rewrite a bunch of other classes you'd never imagined were tied together by a public field.

That's why I told you on IRC that a code cleanup might not be worth it. All this will be rewritten after Luna, and that rewrite already started. This class in particular has been already rewritten in the files linked above.

review: Approve

[Victor Martinez (victored)]

Rico, you bring up an interesting suggestion there. Personally, I'm finding it hard to decide whether I prefer a visually encapsulating namespace ("namespace Noise { ... }") style over simply connecting the dots ("Noise.ClassName")

I think David has the final say on that topic, since it's currently not defined in the current coding style guidelines.

=== modified file 'src/GStreamer/GStreamerTagger.vala'
--- src/GStreamer/GStreamerTagger.vala	2012-10-06 03:06:08 +0000
+++ src/GStreamer/GStreamerTagger.vala	2012-10-06 18:22:21 +0000
@@ -25,149 +25,147 @@
 
 public class Noise.GStreamerTagger : GLib.Object {
 
-        LibraryManager lm;
-        static int DISCOVER_SET_SIZE = 50;
-        Gst.Discoverer d;
-        HashMap<string, int> uri_to_id;
-        LinkedList<string> path_queue;
-
-        public signal void media_imported(Media m);
-        public signal void import_error(string file);
-        public signal void queue_finished();
-
-        bool cancelled;
-
-        public GStreamerTagger(LibraryManager lm) {
-                this.lm = lm;
+    LibraryManager lm;
+    static int DISCOVER_SET_SIZE = 50;
+    Gst.Discoverer d;
+    HashMap<string, int> uri_to_id;
+    LinkedList<string> path_queue;
+
+    public signal void media_imported (Media m);
+    public signal void import_error (string file);
+    public signal void queue_finished();
+
+    bool cancelled;
+
+    public GStreamerTagger (LibraryManager library_manager) {
+        this.lm = library_manager;
 
         d = create_discoverer ();
 
-
-                uri_to_id = new HashMap<string, int>();
-                path_queue = new LinkedList<string>();
-        }
+        uri_to_id = new HashMap<string, int> ();
+        path_queue = new LinkedList<string> ();
+    }
 
     private Gst.Discoverer? create_discoverer () {
         Gst.Discoverer? discoverer = null;
 
-                try {
-                        discoverer = new Gst.Discoverer ((ClockTime)(30*Gst.SECOND));
-                }
-                catch (Error err) {
-                        critical ("Metadata reader could not create discoverer object: %s\n", err.message);
-                }
+        try {
+            discoverer = new Gst.Discoverer ((ClockTime) (30 * Gst.SECOND));
+        } catch (Error err) {
+            critical ("Metadata reader could not create discoverer object: %s\n", err.message);
+        }
 
-                discoverer.discovered.connect (import_media);
-                discoverer.finished.connect (finished);
+        discoverer.discovered.connect (import_media);
+        discoverer.finished.connect (finished);
 
         return discoverer;
     }
 
-        void finished() {
-                if(!cancelled && path_queue.size > 0) {
-                        d = create_discoverer ();
-
-                        d.start();
-                        for(int i = 0; i < DISCOVER_SET_SIZE && i < path_queue.size; ++i) {
-                                d.discover_uri_async("file://" + path_queue.get(i));
-                        }
-                }
-                else {
-                        debug("queue finished\n");
-                        queue_finished();
-                }
-        }
-
-        public void cancel_operations() {
-                cancelled = true;
-        }
-
-        public void discoverer_import_media (LinkedList<string> files) {
-                int size = 0;
-                cancelled = false;
-                path_queue.clear();
-
-                foreach(string s in files) {
-                        path_queue.add(s);
-
-                        d.start();
-                        if(size < DISCOVER_SET_SIZE) {
-                                ++size;
-                                d.discover_uri_async("file://" + s);
-                        }
-                }
-        }
-
-        private async void import_media(DiscovererInfo info, Error err) {
-                path_queue.remove(info.get_uri().replace("file://",""));
+    void finished () {
+        if (!cancelled && path_queue.size > 0) {
+            d = create_discoverer ();
+
+            d.start ();
+            for (int i = 0; i < DISCOVER_SET_SIZE && i < path_queue.size; ++i) {
+                d.discover_uri_async ("file://" + path_queue.get (i));
+            }
+        } else {
+            debug ("queue finished\n");
+            queue_finished ();
+        }
+    }
+
+    public void cancel_operations () {
+        cancelled = true;
+    }
+
+    public void discoverer_import_media (LinkedList<string> files) {
+        int size = 0;
+        cancelled = false;
+        path_queue.clear ();
+
+        foreach (string s in files) {
+            path_queue.add (s);
+
+            d.start ();
+            if (size < DISCOVER_SET_SIZE) {
+                ++size;
+                d.discover_uri_async ("file://" + s);
+            }
+        }
+    }
+
+    private async void import_media (DiscovererInfo info, Error err) {
+        path_queue.remove (info.get_uri ().replace ("file://", ""));
 
         Media? s = null;
 
-                if(info != null && info.get_tags() != null) {
-                        s = new Media(info.get_uri());
-
-                        string title = "";
-                        string artist, composer, album_artist, album, grouping, genre, comment, lyrics;
-                        uint track, track_count, album_number, album_count, bitrate, rating;
-                        double bpm;
-                        GLib.Date? date = GLib.Date();
-
-                        // get title, artist, album artist, album, genre, comment, lyrics strings
-                        if(info.get_tags().get_string(TAG_TITLE, out title))
-                                s.title = title;
-                        if(info.get_tags().get_string(TAG_ARTIST, out artist))
-                                s.artist = artist;
-                        if(info.get_tags().get_string(TAG_COMPOSER, out composer))
-                                s.composer = composer;
-
-                        if(info.get_tags().get_string(TAG_ALBUM_ARTIST, out album_artist))
-                                s.album_artist = album_artist;
-                        else
-                                s.album_artist = s.artist;
-
-                        if(info.get_tags().get_string(TAG_ALBUM, out album))
-                                s.album = album;
-                        if(info.get_tags().get_string(TAG_GROUPING, out grouping))
-                                s.grouping = grouping;
-                        if(info.get_tags().get_string(TAG_GENRE, out genre))
-                                s.genre = genre;
-                        if(info.get_tags().get_string(TAG_COMMENT, out comment))
-                                s.comment = comment;
-                        if(info.get_tags().get_string(TAG_LYRICS, out lyrics))
-                                s.lyrics = lyrics;
-
-                        // get the year
-                        if(info.get_tags().get_date(TAG_DATE, out date)) {
-                                if(date != null)
-                                        s.year = (int)date.get_year();
-                        }
-                        // get track/album number/count, bitrating, rating, bpm
-                        if(info.get_tags().get_uint(TAG_TRACK_NUMBER, out track))
-                                s.track = (int)track;
-                        if(info.get_tags().get_uint(TAG_TRACK_COUNT, out track_count))
-                                s.track_count = track_count;
-
-                        if(info.get_tags().get_uint(TAG_ALBUM_VOLUME_NUMBER, out album_number))
-                                s.album_number = album_number;
-                        if(info.get_tags().get_uint(TAG_ALBUM_VOLUME_COUNT, out album_count))
-                                s.album_count = album_count;
-
-                        if(info.get_tags().get_uint(TAG_BITRATE, out bitrate))
-                                s.bitrate = (int)(bitrate/1000);
-                        if(info.get_tags().get_uint(TAG_USER_RATING, out rating))
-                                s.rating = (int)((rating > 0 && rating <= 5) ? rating : 0);
-                        if(info.get_tags().get_double(TAG_BEATS_PER_MINUTE, out bpm))
-                                s.bpm = (int)bpm;
-
-
-                        // get length
+        if (info != null && info.get_tags () != null) {
+            s = new Media (info.get_uri ());
+
+            string title = "";
+            string artist, composer, album_artist, album, grouping, genre, comment, lyrics;
+            uint track, track_count, album_number, album_count, bitrate, rating;
+            double bpm;
+            GLib.Date? date = GLib.Date ();
+
+            // get title, artist, album artist, album, genre, comment, lyrics strings
+            if (info.get_tags ().get_string (TAG_TITLE, out title))
+                s.title = title;
+            if (info.get_tags ().get_string (TAG_ARTIST, out artist))
+                s.artist = artist;
+            if (info.get_tags ().get_string (TAG_COMPOSER, out composer))
+                s.composer = composer;
+
+            if (info.get_tags ().get_string (TAG_ALBUM_ARTIST, out album_artist))
+                s.album_artist = album_artist;
+            else
+                s.album_artist = s.artist;
+
+            if (info.get_tags ().get_string (TAG_ALBUM, out album))
+                s.album = album;
+            if (info.get_tags ().get_string (TAG_GROUPING, out grouping))
+                s.grouping = grouping;
+            if (info.get_tags ().get_string (TAG_GENRE, out genre))
+                s.genre = genre;
+            if (info.get_tags ().get_string (TAG_COMMENT, out comment))
+                s.comment = comment;
+            if (info.get_tags ().get_string (TAG_LYRICS, out lyrics))
+                s.lyrics = lyrics;
+
+            /* Get the year */
+            if (info.get_tags ().get_date (TAG_DATE, out date)) {
+                if (date != null)
+                    s.year = (int) date.get_year ();
+            }
+
+            /* get track/album number/count, bitrating, rating, bpm */
+            if (info.get_tags ().get_uint (TAG_TRACK_NUMBER, out track))
+                s.track = (int)track;
+            if (info.get_tags ().get_uint (TAG_TRACK_COUNT, out track_count))
+                s.track_count = track_count;
+
+            if (info.get_tags ().get_uint (TAG_ALBUM_VOLUME_NUMBER, out album_number))
+                s.album_number = album_number;
+            if (info.get_tags ().get_uint (TAG_ALBUM_VOLUME_COUNT, out album_count))
+                s.album_count = album_count;
+
+            if (info.get_tags ().get_uint (TAG_BITRATE, out bitrate))
+                s.bitrate = (int) (bitrate / 1000);
+            if (info.get_tags ().get_uint (TAG_USER_RATING, out rating))
+                s.rating = (int) ((rating > 0 && rating <= 5) ? rating : 0);
+            if (info.get_tags ().get_double (TAG_BEATS_PER_MINUTE, out bpm))
+                s.bpm = (int) bpm;
+
+
+            /* Get length */
             uint64 duration = info.get_duration ();
 
             if (duration == 0)
-                        info.get_tags ().get_uint64 (TAG_DURATION, out duration);
+                info.get_tags ().get_uint64 (TAG_DURATION, out duration);
 
-            // we convert from nanoseconds (10E-9) to miliseconds (10E-3);
-            s.length = (uint)((duration * Numeric.MILI_INV) / Numeric.NANO_INV);
+            /* Convert from nanoseconds (10E-9) to miliseconds (10E-3) */
+            s.length = (uint) ((duration * Numeric.MILI_INV) / Numeric.NANO_INV);
 
             foreach (var audio_stream in info.get_audio_streams ()) {
                 if (audio_stream == null)
@@ -182,70 +180,68 @@
                 if (s.bitrate == 0) {
                     debug ("Getting bitrate from stream info");
                     s.bitrate = audio_stream.get_bitrate ();
-                    debug ("Bitrate = %s", s.bitrate.to_string());
+                    debug ("Bitrate = %s", s.bitrate.to_string ());
                 }
 
                 break;
             }
 
-            // Get cover art
-                        import_art_async.begin (s, info);
-                }
-                else {
-                        s = taglib_import_media(info.get_uri());
-
-                if (s == null) {
-                        import_error (info.get_uri().replace("file://", ""));
-                        return;
-            }
-                }
-
-                // get the size
-                s.file_size = yield FileUtils.get_size_async (s.file);
-                s.date_added = (int)time_t();
-
-                media_imported(s);
-        }
-
-        public Media? taglib_import_media(string uri) {
-                Media s = new Media(uri);
-                TagLib.File tag_file;
-
-                tag_file = new TagLib.File(uri.replace("file://",""));
-
-                if(tag_file != null && tag_file.tag != null && tag_file.audioproperties != null) {
-                        try {
-                                s.title = tag_file.tag.title;
-                                s.artist = tag_file.tag.artist;
-                                s.album = tag_file.tag.album;
-                                s.genre = tag_file.tag.genre;
-                                s.comment = tag_file.tag.comment;
-                                s.year = (int)tag_file.tag.year;
-                                s.track = (int)tag_file.tag.track;
-                                s.bitrate = tag_file.audioproperties.bitrate;
-
-                                s.length = (uint)(tag_file.audioproperties.length * Numeric.MILI_INV);
-                                s.samplerate = tag_file.audioproperties.samplerate;
-                        }
-                        finally {
-                                if(s.title == null || s.title == "") {
-                                        string[] paths = uri.split("/", 0);
-                                        s.title = paths[paths.length - 1];
-                                }
-                                if(s.artist == null || s.artist == "") s.artist = Media.UNKNOWN_ARTIST;
-
-                                s.album_artist = s.artist;
-                                s.album_number = 1;
-                        }
-                }
-                else {
-                        return null;
-                }
-
-                return s;
-        }
-
-        private async void import_art_async (Media m, DiscovererInfo info) {
+            /* Get cover art */
+            import_art_async.begin (s, info);
+        } else {
+            s = taglib_import_media (info.get_uri ());
+
+            if (s == null) {
+                import_error (info.get_uri ().replace ("file://", ""));
+                return;
+            }
+        }
+
+        /* Get the size */
+        s.file_size = yield FileUtils.get_size_async (s.file);
+        s.date_added = (int) time_t ();
+
+        media_imported (s);
+    }
+
+    public Media? taglib_import_media (string uri) {
+        Media s = new Media (uri);
+        TagLib.File tag_file;
+
+        tag_file = new TagLib.File (uri.replace ("file://",""));
+
+        if (tag_file != null && tag_file.tag != null && tag_file.audioproperties != null) {
+            try {
+                s.title = tag_file.tag.title;
+                s.artist = tag_file.tag.artist;
+                s.album = tag_file.tag.album;
+                s.genre = tag_file.tag.genre;
+                s.comment = tag_file.tag.comment;
+                s.year = (int) tag_file.tag.year;
+                s.track = (int) tag_file.tag.track;
+                s.bitrate = tag_file.audioproperties.bitrate;
+
+                s.length = (uint) (tag_file.audioproperties.length * Numeric.MILI_INV);
+                s.samplerate = tag_file.audioproperties.samplerate;
+            } finally {
+                if (s.title == null || s.title == "") {
+                    string[] paths = uri.split ("/", 0);
+                    s.title = paths[paths.length - 1];
+                }
+
+                if (s.artist == null || s.artist == "") s.artist = Media.UNKNOWN_ARTIST;
+
+                s.album_artist = s.artist;
+                s.album_number = 1;
+            }
+        } else {
+            return null;
+        }
+
+        return s;
+    }
+
+    private async void import_art_async (Media m, DiscovererInfo info) {
         var cache = CoverartCache.instance;
 
         if (cache.has_image (m))
@@ -257,7 +253,7 @@
             yield cache.cache_image_async (m, pix);
         else
             warning ("import_art: null pixbuf");
-        }
+    }
 
     private static Gdk.Pixbuf? get_image (Gst.TagList tag) {
         Gst.Buffer? buffer = null;
@@ -275,14 +271,14 @@
                 continue;
 
             int image_type;
-                        structure.get_enum ("image-type", typeof (Gst.TagImageType), out image_type);
+            structure.get_enum ("image-type", typeof (Gst.TagImageType), out image_type);
 
-                        if (image_type == Gst.TagImageType.FRONT_COVER) {
-                                buffer = loop_buffer;
-                                break;
-                        } else if (image_type == Gst.TagImageType.UNDEFINED || buffer == null) {
-                buffer = loop_buffer;
-                        }
+            if (image_type == Gst.TagImageType.FRONT_COVER) {
+                buffer = loop_buffer;
+                break;
+            } else if (image_type == Gst.TagImageType.UNDEFINED || buffer == null) {
+                buffer = loop_buffer;
+            }
         }
 
         if (buffer == null) {
@@ -308,99 +304,99 @@
         return pix;
     }
 
-        public bool save_media(Media s) {
-                return false;
-
-                /*Gst.Pipeline pipe = new Pipeline("pipe");
-                Element src = Element.make_from_uri(URIType.SRC, "file://" + s.file, null);
-                Element decoder = ElementFactory.make("decodebin", "decoder");
-
-                GLib.Signal.connect(decoder, "new-decoded-pad", (GLib.Callback)newDecodedPad, this);
-
-                if(!((Gst.Bin)pipe).add_many(src, decoder)) {
-                        message ("Could not add src and decoder to pipeline to save metadata\n");
-                        return false;
-                }
-
-                if(!src.link_many(decoder)) {
-                        message ("Could not link src to decoder to save metadata\n");
-                        return false;
-                }
-
-
-                Gst.Element queue = ElementFactory.make("queue", "queue");
-                Gst.Element queue2 = ElementFactory.make("queue", "queue2");
-
-                if(queue == null || queue2 == null) {
-                        message ("could not add create queues to save metadata\n");
-                        return false;
-                }
-
-                if(!((Gst.Bin)pipe).add_many(queue, queue2)) {
-                        warning ("Could not add queue's to save metadata\n");
-                        return false;
-                }
-
-                queue.set("max-size-time", 120 * Gst.SECOND);
-
-
-
-
-                //Element encoder = new_element_from_uri(URIType.SINK, "file://" + s.file, null);
-
-                Gst.TagList tags;
-                bool rv = true;
-                //long day;
-
-                tags = new TagList();
-                tags.add(TagMergeMode.REPLACE,  TAG_TITLE, s.title,
-                                                                                TAG_ARTIST, s.artist,
-                                                                                TAG_COMPOSER, s.composer,
-                                                                                TAG_ALBUM_ARTIST, s.album_artist,
-                                                                                TAG_ALBUM, s.album,
-                                                                                TAG_GROUPING, s.grouping,
-                                                                                TAG_GENRE, s.genre,
-                                                                                TAG_COMMENT, s.comment,
-                                                                                TAG_LYRICS, s.lyrics,
-                                                                                TAG_TRACK_NUMBER, s.track,
-                                                                                TAG_TRACK_COUNT, s.track_count,
-                                                                                TAG_ALBUM_VOLUME_NUMBER, s.album_number,
-                                                                                TAG_ALBUM_VOLUME_COUNT, s.album_count,
-                                                                                TAG_USER_RATING, s.rating);
-
-                /* fetch date, set new year to s.year, set date */
-
-                // now find a tag setter interface and use it
-                /*Gst.Iterator iter;
-                bool done;
-
-                iter = ((Gst.Bin)pipeline).iterate_all_by_interface(typeof(Gst.TagSetter));
-                done = false;
-                while (!done) {
-                          Gst.TagSetter tagger = null;
-
-                          switch (iter.next(out tagger) {
-                          case GST_ITERATOR_OK:
-                                        tagger.merge_tags (tags, GST_TAG_MERGE_REPLACE_ALL);
-                                        break;
-                          case GST_ITERATOR_RESYNC:
-                                        iter.resync();
-                                        break;
-                          case GST_ITERATOR_ERROR:
-                                        warning("Could not update metadata on media\n");
-                                        rv = false;
-                                        done = true;
-                                        break;
-                          case GST_ITERATOR_DONE:
-                                        done = true;
-                                        break;
-                          }
-                }
-
-                return rv;      */
-        }
-
-        public bool save_embeddeart_d(Gdk.Pixbuf pix) {
-                return false;
-        }
+    public bool save_media (Media s) {
+        return false;
+
+        /*Gst.Pipeline pipe = new Pipeline("pipe");
+          Element src = Element.make_from_uri(URIType.SRC, "file://" + s.file, null);
+          Element decoder = ElementFactory.make("decodebin", "decoder");
+
+          GLib.Signal.connect(decoder, "new-decoded-pad", (GLib.Callback)newDecodedPad, this);
+
+          if(!((Gst.Bin)pipe).add_many(src, decoder)) {
+          message ("Could not add src and decoder to pipeline to save metadata\n");
+          return false;
+          }
+
+          if(!src.link_many(decoder)) {
+          message ("Could not link src to decoder to save metadata\n");
+          return false;
+          }
+
+
+          Gst.Element queue = ElementFactory.make("queue", "queue");
+          Gst.Element queue2 = ElementFactory.make("queue", "queue2");
+
+          if(queue == null || queue2 == null) {
+          message ("could not add create queues to save metadata\n");
+          return false;
+          }
+
+          if(!((Gst.Bin)pipe).add_many(queue, queue2)) {
+          warning ("Could not add queue's to save metadata\n");
+          return false;
+          }
+
+          queue.set("max-size-time", 120 * Gst.SECOND);
+
+
+
+
+          //Element encoder = new_element_from_uri(URIType.SINK, "file://" + s.file, null);
+
+          Gst.TagList tags;
+          bool rv = true;
+          //long day;
+
+          tags = new TagList();
+          tags.add(TagMergeMode.REPLACE,  TAG_TITLE, s.title,
+          TAG_ARTIST, s.artist,
+          TAG_COMPOSER, s.composer,
+          TAG_ALBUM_ARTIST, s.album_artist,
+          TAG_ALBUM, s.album,
+          TAG_GROUPING, s.grouping,
+          TAG_GENRE, s.genre,
+          TAG_COMMENT, s.comment,
+          TAG_LYRICS, s.lyrics,
+          TAG_TRACK_NUMBER, s.track,
+          TAG_TRACK_COUNT, s.track_count,
+          TAG_ALBUM_VOLUME_NUMBER, s.album_number,
+          TAG_ALBUM_VOLUME_COUNT, s.album_count,
+          TAG_USER_RATING, s.rating);
+
+          /* fetch date, set new year to s.year, set date */
+
+        // now find a tag setter interface and use it
+        /*Gst.Iterator iter;
+          bool done;
+
+          iter = ((Gst.Bin)pipeline).iterate_all_by_interface(typeof(Gst.TagSetter));
+          done = false;
+          while (!done) {
+          Gst.TagSetter tagger = null;
+
+          switch (iter.next(out tagger) {
+          case GST_ITERATOR_OK:
+          tagger.merge_tags (tags, GST_TAG_MERGE_REPLACE_ALL);
+          break;
+          case GST_ITERATOR_RESYNC:
+          iter.resync();
+          break;
+          case GST_ITERATOR_ERROR:
+          warning("Could not update metadata on media\n");
+          rv = false;
+          done = true;
+          break;
+          case GST_ITERATOR_DONE:
+          done = true;
+          break;
+          }
+          }
+
+          return rv;      */
+    }
+
+    public bool save_embeddeart_d (Gdk.Pixbuf pix) {
+        return false;
+    }
 }