Facebook problem
Dirk Hohndel
dirk at hohndel.org
Sat Oct 15 12:41:07 PDT 2016
Hi Edi,
Thanks for sending patches for this. Really appreciated.
I have some feedback for you that I'd love for you to take a look at...
On Sat, Oct 15, 2016 at 04:53:36AM +0000, Edi Anderson wrote:
> Hi guys, i saw the problem listed in bugtracker (#1101) and I am sending
> some patchs.
> From 49ecb1c6f3a07a3733c20b808433711c5ec7d420 Mon Sep 17 00:00:00 2001
> From: Edi Anderson Lobo <ediandersonlobo at gmail.com>
> Date: Sat, 15 Oct 2016 00:33:08 -0300
> Subject: [PATCH 3/4] Changed message.
>
> I changed the message of success on login with facebook.
This commit message and the patch that goes along with it don't seem to
match. All the patch does is add a new private method in the header
file...
> diff --git a/desktop-widgets/plugins/facebook/facebookconnectwidget.h b/desktop-widgets/plugins/facebook/facebookconnectwidget.h
> index e970978..91208cd 100644
> --- a/desktop-widgets/plugins/facebook/facebookconnectwidget.h
> +++ b/desktop-widgets/plugins/facebook/facebookconnectwidget.h
> @@ -50,6 +50,10 @@ public:
> SocialNetworkDialog(QWidget *parent = 0);
> QString text() const;
> QString album() const;
> +
> +private:
> + void shallEnableCheckBoxes() ;
This has white space issues. We indent with TAB (see CodingStyle)
> public slots:
> void selectionChanged();
> void albumChanged();
> From 50ccdba674498cec66d4beb9643f831cc6145088 Mon Sep 17 00:00:00 2001
> From: Edi Anderson Lobo <ediandersonlobo at gmail.com>
> Date: Fri, 14 Oct 2016 23:49:06 -0300
> Subject: [PATCH 1/4] Checkbox disabled.
>
> I disabled the checkbox to activate them during the runtime.
> So when the data associated with checkbox is not null, the
> checkbox is enabled. So null data will not be sent to facebook
[...]
This one looked good
> From 8a136d88089390d20ff6ea713e2e21140e012531 Mon Sep 17 00:00:00 2001
> From: Edi Anderson Lobo <ediandersonlobo at gmail.com>
> Date: Sat, 15 Oct 2016 00:45:04 -0300
> Subject: [PATCH 4/4] Added validation.
>
> Added an new method for validate the
> values to be sending for facebook.
>
> Signed-off-by: Edi Anderson Lobo <ediandersonlobo at gmail.com>
> ---
> .../plugins/facebook/facebookconnectwidget.cpp | 78 ++++++++++++++--------
> 1 file changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/desktop-widgets/plugins/facebook/facebookconnectwidget.cpp b/desktop-widgets/plugins/facebook/facebookconnectwidget.cpp
> index e0d2aab..d976c45 100644
> --- a/desktop-widgets/plugins/facebook/facebookconnectwidget.cpp
> +++ b/desktop-widgets/plugins/facebook/facebookconnectwidget.cpp
> @@ -160,8 +160,9 @@ void FacebookManager::setDesiredAlbumName(const QString& a)
> void FacebookManager::sendDive()
> {
> SocialNetworkDialog dialog(qApp->activeWindow());
> - if (dialog.exec() != QDialog::Accepted)
> + if (dialog.exec() != QDialog::Accepted){
More whitespace issues - I know, this may seem silly, but please look at
CodingStyle regarding indentation and adding a space between ')' and '{'
here.
> @@ -237,10 +238,13 @@ FacebookConnectWidget::FacebookConnectWidget(QWidget *parent) : QDialog(parent),
>
> void FacebookConnectWidget::facebookLoggedIn()
> {
> - ui->fbWebviewContainer->hide();
> - ui->fbWebviewContainer->setEnabled(false);
> - ui->FBLabel->setText(tr("To disconnect Subsurface from your Facebook account, use the button below"));
> -}
> + hide();
> + QMessageBox::information(this,
> + tr("Facebook Login"),
> + tr("Your loggin on Facebook was realized with sucess."),
I'm not a native speaker of English, either, but how about "You have
successfully logged into Facebook." ?
> @@ -268,6 +272,25 @@ SocialNetworkDialog::SocialNetworkDialog(QWidget *parent) :
> connect(ui->Location, SIGNAL(clicked()), this, SLOT(selectionChanged()));
> connect(ui->Notes, SIGNAL(clicked()), this, SLOT(selectionChanged()));
> connect(ui->album, SIGNAL(textChanged(QString)), this, SLOT(albumChanged()));
> + shallEnableCheckBoxes();
> +}
> +
> +void SocialNetworkDialog::shallEnableCheckBoxes()
> +{
> + struct dive *d = current_dive;
> + if(d != NULL){
> + ui->duration->setEnabled(true);
> + if (get_short_dive_date_string(d->when) != "" )
> + ui->date->setEnabled(true);
> + if (get_dive_location(d) != NULL)
> + ui->Location->setEnabled(true);
> + if (d->buddy != NULL)
> + ui->Buddy->setEnabled(true);
> + if (d->divemaster != NULL)
> + ui->Divemaster->setEnabled(true);
> + if ( d->notes != NULL)
> + ui->Notes->setEnabled(true);
> + }
This all needs some whitespace love. I assume you are using some IDE?
Please ensure that it's set up to use TAB for indentation.
> @@ -279,27 +302,28 @@ void SocialNetworkDialog::albumChanged()
> void SocialNetworkDialog::selectionChanged()
> {
> struct dive *d = current_dive;
> - QString fullText;
> - if (ui->date->isChecked()) {
> - fullText += tr("Dive date: %1 \n").arg(get_short_dive_date_string(d->when));
> - }
> - if (ui->duration->isChecked()) {
> - fullText += tr("Duration: %1 \n").arg(get_dive_duration_string(d->duration.seconds,
> - tr("h:", "abbreviation for hours plus separator"),
> - tr("min", "abbreviation for minutes")));
> - }
> - if (ui->Location->isChecked()) {
> - fullText += tr("Dive location: %1 \n").arg(get_dive_location(d));
> - }
> - if (ui->Buddy->isChecked()) {
> - fullText += tr("Buddy: %1 \n").arg(d->buddy);
> - }
> - if (ui->Divemaster->isChecked()) {
> - fullText += tr("Divemaster: %1 \n").arg(d->divemaster);
> - }
> - if (ui->Notes->isChecked()) {
> - fullText += tr("\n%1").arg(d->notes);
> - }
> + QString fullText;
> +
> + if (ui->date->isChecked()) {
> + fullText += tr("Dive date: %1 \n").arg(get_short_dive_date_string(d->when) );
> + }
> + if (ui->duration->isChecked()) {
> + fullText += tr("Duration: %1 \n").arg(get_dive_duration_string(d->duration.seconds,
> + tr("h:", "abbreviation for hours plus separator"),
> + tr("min", "abbreviation for minutes")));
> + }
> + if (ui->Location->isChecked()) {
> + fullText += tr("Dive location: %1 \n").arg(get_dive_location(d));
> + }
> + if (ui->Buddy->isChecked()) {
> + fullText += tr("Buddy: %1 \n").arg(d->buddy);
> + }
> + if (ui->Divemaster->isChecked()) {
> + fullText += tr("Divemaster: %1 \n").arg(d->divemaster);
> + }
> + if (ui->Notes->isChecked()) {
> + fullText += tr("\n%1").arg(d->notes);
> + }
> ui->text->setPlainText(fullText);
It's a little hard to tell, but this seems to be nothing but an incorrect
whitespace change?
> From b6901d1fc194932441f75a50e561bd8c6aa1e45a Mon Sep 17 00:00:00 2001
> From: Edi Anderson Lobo <ediandersonlobo at gmail.com>
> Date: Sat, 15 Oct 2016 00:09:10 -0300
> Subject: [PATCH 2/4] Call for sending data.
>
> Added an call for function sendDive,for send data
> for facebook. I changed the way how the widget
> FacebookConnectWidget is allocated. When the user
> make loggin in facebook and after loggof
> and try loggin again get an error.
I have a hard time understanding this commit message. Please try to
describe the substance of your change. So instead of "I changed the way
the widget is allocated" explain HOW the widget is allocated now (and why
that's better than was done before).
>
> Reported-by: BenjaminF -- Bugtracker #1101.
We have really cool feature in our tooling... if you add
Fixes: #1101
to a commit message, the bug will automatically be closed with a reference
to your commit...
> diff --git a/desktop-widgets/plugins/facebook/facebook_integration.cpp b/desktop-widgets/plugins/facebook/facebook_integration.cpp
> index 58d757e..5611d2d 100644
> --- a/desktop-widgets/plugins/facebook/facebook_integration.cpp
> +++ b/desktop-widgets/plugins/facebook/facebook_integration.cpp
> @@ -1,13 +1,13 @@
> #include "facebook_integration.h"
> #include "facebookconnectwidget.h"
> +#include <QMessageBox>
> +#include <QScopedPointer>
>
> #include <QDebug>
>
> -FacebookPlugin::FacebookPlugin(QObject* parent) :
> - fbConnectWidget(new FacebookConnectWidget()),
> - fbUploadDialog(new SocialNetworkDialog())
> +FacebookPlugin::FacebookPlugin(QObject* parent)
> {
> - Q_UNUSED(parent)
> + Q_UNUSED(parent)
So besides the broken whitespace, this removes the creation of the two
widgets at FacebookPlugin creation time.
You create the FacebookConnectWidget below, but I don't see where the
SocialNetworkDialog widget is created. Is this not needed anymore?
> @@ -18,7 +18,8 @@ bool FacebookPlugin::isConnected()
>
> void FacebookPlugin::requestLogin()
> {
> - fbConnectWidget->exec();
> + QScopedPointer<FacebookConnectWidget> fbConnectWidget(new FacebookConnectWidget());
> + fbConnectWidget->exec();
Now every time we do "requestLogin()", a new widget is created - if we had
one before, it doesn't get destroyed / freed.
> @@ -39,6 +40,6 @@ QString FacebookPlugin::socialNetworkName() const
> void FacebookPlugin::requestUpload()
> {
> FacebookManager *instance = FacebookManager::instance();
> - if (instance->loggedIn())
> - fbUploadDialog->exec();
> + if ( instance->loggedIn() )
> + instance->sendDive();
> }
> --
This could use more explanation.
/D
More information about the subsurface
mailing list