| Message ID | 20200430141433.6776-2-email@uajain.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 30/04/2020 15:15, Umang Jain wrote: > This commit introduces no functional changes. > This is required so that the combo-box list can be managed > conveniently from various private functions in subsequent > commit. > > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/qcam/main_window.cpp | 9 ++++----- > src/qcam/main_window.h | 2 ++ > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index d021fa9..344b7ec 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -11,7 +11,6 @@ > #include <string> > #include <sys/mman.h> > > -#include <QComboBox> Shouldn't we keep this here ... and... > #include <QCoreApplication> > #include <QFileDialog> > #include <QImage> > @@ -114,14 +113,14 @@ int MainWindow::createToolbars() > connect(action, &QAction::triggered, this, &MainWindow::quit); > > /* Camera selector. */ > - QComboBox *cameraCombo = new QComboBox(); > - connect(cameraCombo, QOverload<int>::of(&QComboBox::activated), > + cameraCombo_ = new QComboBox(); > + connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated), > this, &MainWindow::switchCamera); > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > - cameraCombo->addItem(QString::fromStdString(cam->name())); > + cameraCombo_->addItem(QString::fromStdString(cam->name())); > > - toolbar_->addWidget(cameraCombo); > + toolbar_->addWidget(cameraCombo_); > > toolbar_->addSeparator(); > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index 5d6251c..28c325f 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -9,6 +9,7 @@ > > #include <memory> > > +#include <QComboBox> instead just forward declare class QComboBox; But as this hasn't been done for other parts of this component, I don't think it makes much difference. Either way... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > #include <QElapsedTimer> > #include <QIcon> > #include <QMainWindow> > @@ -72,6 +73,7 @@ private: > /* UI elements */ > QToolBar *toolbar_; > QAction *startStopAction_; > + QComboBox *cameraCombo_; > ViewFinder *viewfinder_; > > QIcon iconPlay_; >
On Thu, Apr 30, 2020 at 04:05:56PM +0100, Kieran Bingham wrote: > On 30/04/2020 15:15, Umang Jain wrote: > > This commit introduces no functional changes. > > This is required so that the combo-box list can be managed > > conveniently from various private functions in subsequent > > commit. > > > > Signed-off-by: Umang Jain <email@uajain.com> > > --- > > src/qcam/main_window.cpp | 9 ++++----- > > src/qcam/main_window.h | 2 ++ > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index d021fa9..344b7ec 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -11,7 +11,6 @@ > > #include <string> > > #include <sys/mman.h> > > > > -#include <QComboBox> > > Shouldn't we keep this here ... and... > > > #include <QCoreApplication> > > #include <QFileDialog> > > #include <QImage> > > @@ -114,14 +113,14 @@ int MainWindow::createToolbars() > > connect(action, &QAction::triggered, this, &MainWindow::quit); > > > > /* Camera selector. */ > > - QComboBox *cameraCombo = new QComboBox(); > > - connect(cameraCombo, QOverload<int>::of(&QComboBox::activated), > > + cameraCombo_ = new QComboBox(); > > + connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated), > > this, &MainWindow::switchCamera); > > > > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > - cameraCombo->addItem(QString::fromStdString(cam->name())); > > + cameraCombo_->addItem(QString::fromStdString(cam->name())); > > > > - toolbar_->addWidget(cameraCombo); > > + toolbar_->addWidget(cameraCombo_); > > > > toolbar_->addSeparator(); > > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index 5d6251c..28c325f 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -9,6 +9,7 @@ > > > > #include <memory> > > > > +#include <QComboBox> > > instead just forward declare > > class QComboBox; > > But as this hasn't been done for other parts of this component, I don't > think it makes much difference. > > Either way... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I was going to mention the same. We already forward-declare QAction that way. QToolBar should do the same, and so should QComboBox here. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > #include <QElapsedTimer> > > #include <QIcon> > > #include <QMainWindow> > > @@ -72,6 +73,7 @@ private: > > /* UI elements */ > > QToolBar *toolbar_; > > QAction *startStopAction_; > > + QComboBox *cameraCombo_; > > ViewFinder *viewfinder_; > > > > QIcon iconPlay_;
Hi all, On Thu, Apr 30, 2020 at 18:18, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thu, Apr 30, 2020 at 04:05:56PM +0100, Kieran Bingham wrote: >> On 30/04/2020 15:15, Umang Jain wrote: >> > This commit introduces no functional changes. >> > This is required so that the combo-box list can be managed >> > conveniently from various private functions in subsequent >> > commit. >> > >> > Signed-off-by: Umang Jain <email@uajain.com >> <mailto:email@uajain.com>> >> > --- >> > src/qcam/main_window.cpp | 9 ++++----- >> > src/qcam/main_window.h | 2 ++ >> > 2 files changed, 6 insertions(+), 5 deletions(-) >> > >> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp >> > index d021fa9..344b7ec 100644 >> > --- a/src/qcam/main_window.cpp >> > +++ b/src/qcam/main_window.cpp >> > @@ -11,7 +11,6 @@ >> > #include <string> >> > #include <sys/mman.h> >> > >> > -#include <QComboBox> >> >> Shouldn't we keep this here ... and... >> >> > #include <QCoreApplication> >> > #include <QFileDialog> >> > #include <QImage> >> > @@ -114,14 +113,14 @@ int MainWindow::createToolbars() >> > connect(action, &QAction::triggered, this, &MainWindow::quit); >> > >> > /* Camera selector. */ >> > - QComboBox *cameraCombo = new QComboBox(); >> > - connect(cameraCombo, QOverload<int>::of(&QComboBox::activated), >> > + cameraCombo_ = new QComboBox(); >> > + connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated), >> > this, &MainWindow::switchCamera); >> > >> > for (const std::shared_ptr<Camera> &cam : cm_->cameras()) >> > - cameraCombo->addItem(QString::fromStdString(cam->name())); >> > + cameraCombo_->addItem(QString::fromStdString(cam->name())); >> > >> > - toolbar_->addWidget(cameraCombo); >> > + toolbar_->addWidget(cameraCombo_); >> > >> > toolbar_->addSeparator(); >> > >> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h >> > index 5d6251c..28c325f 100644 >> > --- a/src/qcam/main_window.h >> > +++ b/src/qcam/main_window.h >> > @@ -9,6 +9,7 @@ >> > >> > #include <memory> >> > >> > +#include <QComboBox> >> >> instead just forward declare >> >> class QComboBox; >> >> But as this hasn't been done for other parts of this component, I >> don't >> think it makes much difference. >> >> Either way... >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com >> <mailto:kieran.bingham@ideasonboard.com>> > > I was going to mention the same. We already forward-declare QAction > that > way. QToolBar should do the same, and so should QComboBox here. Apart > from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com > <mailto:laurent.pinchart@ideasonboard.com>> Well, the QToolBar is already forward-declared via #include <QMainWindow> header, so it's not should not be necessary to foward-declare it here. Thanks for the comments. I will submit v2 of the patches in a few minutes. > >> > #include <QElapsedTimer> >> > #include <QIcon> >> > #include <QMainWindow> >> > @@ -72,6 +73,7 @@ private: >> > /* UI elements */ >> > QToolBar *toolbar_; >> > QAction *startStopAction_; >> > + QComboBox *cameraCombo_; >> > ViewFinder *viewfinder_; >> > >> > QIcon iconPlay_; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index d021fa9..344b7ec 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -11,7 +11,6 @@ #include <string> #include <sys/mman.h> -#include <QComboBox> #include <QCoreApplication> #include <QFileDialog> #include <QImage> @@ -114,14 +113,14 @@ int MainWindow::createToolbars() connect(action, &QAction::triggered, this, &MainWindow::quit); /* Camera selector. */ - QComboBox *cameraCombo = new QComboBox(); - connect(cameraCombo, QOverload<int>::of(&QComboBox::activated), + cameraCombo_ = new QComboBox(); + connect(cameraCombo_, QOverload<int>::of(&QComboBox::activated), this, &MainWindow::switchCamera); for (const std::shared_ptr<Camera> &cam : cm_->cameras()) - cameraCombo->addItem(QString::fromStdString(cam->name())); + cameraCombo_->addItem(QString::fromStdString(cam->name())); - toolbar_->addWidget(cameraCombo); + toolbar_->addWidget(cameraCombo_); toolbar_->addSeparator(); diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h index 5d6251c..28c325f 100644 --- a/src/qcam/main_window.h +++ b/src/qcam/main_window.h @@ -9,6 +9,7 @@ #include <memory> +#include <QComboBox> #include <QElapsedTimer> #include <QIcon> #include <QMainWindow> @@ -72,6 +73,7 @@ private: /* UI elements */ QToolBar *toolbar_; QAction *startStopAction_; + QComboBox *cameraCombo_; ViewFinder *viewfinder_; QIcon iconPlay_;
This commit introduces no functional changes. This is required so that the combo-box list can be managed conveniently from various private functions in subsequent commit. Signed-off-by: Umang Jain <email@uajain.com> --- src/qcam/main_window.cpp | 9 ++++----- src/qcam/main_window.h | 2 ++ 2 files changed, 6 insertions(+), 5 deletions(-)