[Bug 643403] Add support for GStreamer tuner interface (patch)

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=643403
  GStreamer | qt-gstreamer | git

George Kiagiadakis <kiagiadakis.george> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #182034|none                        |needs-work
             status|                            |

--- Comment #1 from George Kiagiadakis <kiagiadakis.george at gmail.com> 2011-03-07 10:59:39 UTC ---
Review of attachment 182034:
 --> (https://bugzilla.gnome.org/review?bug=643403&attachment=182034)

Hello,

Thanks a lot for the patch and sorry for delaying this review. I have made some
comments on some parts of the API which need fixing. The rest seems fine and
I'll happily merge it once those small issues are fixed.

Best regards,
George

::: src/QGst/tuner.h
@@ +20,3 @@
+
+#include "global.h"
+#include "../QGlib/Object"

I'd prefer this to be #include "../QGlib/object.h" for consistency with the
rest of the files.

@@ +31,3 @@
+    QGST_WRAPPER(TunerChannel)
+public:
+    enum TunerChannelFlag {

This enum and its flags declaration should be defined outside of the class
scope, in enums.h

@@ +32,3 @@
+public:
+    enum TunerChannelFlag {
+      Input = 1<<0,

also the names will need to be changed to TunerChannelInput, etc..

@@ +56,3 @@
+public:
+    QString label() const;
+    QGlib::Value framerate() const;

This is a bit weird being a GValue. I suppose that this is always a
GST_TYPE_FRACTION GValue containing the framerate? In this case, it would make
sense to export it as QGst::Fraction instead of QGlib::Value...

@@ +67,3 @@
+public:
+    QList<TunerChannelPtr> channels() const;
+    TunerChannelPtr getChannel() const;

I think the name should be currentChannel() here.

@@ +68,3 @@
+    QList<TunerChannelPtr> channels() const;
+    TunerChannelPtr getChannel() const;
+    void setChannel(const TunerChannelPtr &channel);

...and setCurrentChannel() respectively.

@@ +69,3 @@
+    TunerChannelPtr getChannel() const;
+    void setChannel(const TunerChannelPtr &channel);
+    TunerChannelPtr findChannelByName(const QString &name) const;

Use const char* for string arguments. Channel names are not translatable
strings anyway and in most cases the programmer uses a string literal here
(i.e. findChannelByName("composite")), in which case QString adds an extra
overhead.

@@ +72,3 @@
+
+    QList<TunerNormPtr> norms() const;
+    TunerNormPtr getNorm() const;

Similar to currentChannel(), I think this should be currentNorm()

@@ +73,3 @@
+    QList<TunerNormPtr> norms() const;
+    TunerNormPtr getNorm() const;
+    void setNorm(const TunerNormPtr &norm);

...and setCurrentNorm()

@@ +74,3 @@
+    TunerNormPtr getNorm() const;
+    void setNorm(const TunerNormPtr &norm);
+    TunerNormPtr findNormByName(const QString &name) const;

again, const char*

@@ +77,3 @@
+    
+    unsigned long frequency(const TunerChannelPtr &channel) const;
+    void setFrequency(unsigned long freq, const TunerChannelPtr &channel);

I think the channel argument here should be first, like in the C API.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.