[Differential] [Commented On] D1674: kdev-embedded review

[brauch (Sven Brauch)]

brauch added inline comments.

INLINE COMMENTS

> arduinowindow.cpp:72
>      m_model (new ArduinoWindowModel(parent)),
> +    m_avrdudeProcess (new QProcess(parent)),
>      devices (new Solid::DeviceNotifier)

I'd just put this on the stack

> arduinowindow.cpp:110
> +    buttonBox->button(QDialogButtonBox::Ok)->setEnabled(false);
> +    buttonBox->button(QDialogButtonBox::Ok)->setText("Upload");
> +

i18n

> arduinowindow.cpp:113
> +    // Start output box configuration
> +    output->setTextBackgroundColor(QColor(0, 0, 0, 255));
> +    output->setTextColor(QColor(0, 255, 0, 255));

No fixed colors, sorry. Use the palette. ;)

> arduinowindow.cpp:115
> +    output->setTextColor(QColor(0, 255, 0, 255));
> +    output->append("Welcome,\n\nKDev-Embedded is still in alpha,\nplease be careful and report any problems you find.\n\nHave fun !");
> +

i18n

> arduinowindow.cpp:135
> +{
> +    QString path;
> +    path = QFileDialog::getOpenFileName(this, i18n("Find Files"), QDir::currentPath());

const auto path = ...

> arduinowindow.cpp:136
> +    QString path;
> +    path = QFileDialog::getOpenFileName(this, i18n("Find Files"), QDir::currentPath());
> +    if (!path.isEmpty())

that title is not super great :D

> arduinowindow.cpp:275
> +        << "-c" << Board::instance().m_boards[id].m_upProtocol[0]
> +        << "-P" << "/dev/"+m_interface
> +        << "-b" << Board::instance().m_boards[id].m_upSpeed

Just a note that this is the point where you break e.g. Windows compatiblity. Maybe you find a way to make this also work on windows, at least potentially?

> arduinowindow.cpp:284
> +    output->clear();
> +    output->append("Running...\n");
> +    qCDebug(AwMsg) << QString(arduinoPath+Toolkit::avrdudePath()) << flags;

i18n()

> arduinowindow.cpp:286
> +    qCDebug(AwMsg) << QString(arduinoPath+Toolkit::avrdudePath()) << flags;
> +    m_avrdudeProcess->start(QString(arduinoPath+Toolkit::avrdudePath()),flags);
> +}

missing space after comma

> arduinowindow.cpp:295
> +        qCDebug(AwMsg) << QString("Error during upload.\n" + perr) << exitCode << exitStatus;
> +        output->append(QString("Error during upload. ☹\nCode: %0\n%1").arg(exitCode).arg(perr));
> +    }

i18n

> arduinowindow.cpp:300
> +        qCDebug(AwMsg) << QString("Upload complete.\n" + perr) << exitCode << exitStatus;
> +        output->append(QString("Upload complete. ☺\n%0").arg(perr));
> +    }

i18n

> arduinowindow.cpp:319
> +    bool enable = false;
> +    if(state == Qt::Checked)
> +        enable = true;

always use braces please ... also auto enable = state == Qt::Checked would be simpler imo ;)

> arduinowindow.cpp:343
> +    // TODO: add a better board description
> +    return QString("<p>Processor:</p> \
> +                    <ul>  \

i18n

> arduinowindow.cpp:368
> +    if (list.size() <= 1)
> +        item = "<font color='red'>"+list[0]+"</font>";
> +    else

No fixed colors. Use the palette.

> arduinowindow.h:11
> +#include <QProcess>
> +/*
> +namespace QProcess

don't worry, you can occasionally include something :D

> board.cpp:93
> +{
> +        return QString::number(freq.left(freq.lastIndexOf("0")+1).toInt()/1e6)+"MHz";
> +}

I don't really like this lastIndexOf(0), where is it written that the frequency ends with a 0? it could reasonably be 3999999 or something like that.

> board.cpp:153
>                  m_boards[productId].m_upMaxSize << attrValue;
> +                m_boards[productId].m_upMaxSizeKb << QString::number((attrValue).toInt()/1024);
> +            }

remove ( )

> toolkit.cpp:34
>  
> +/*
> +QStringList Toolkit::avrdudeFlags(const Board *board)

remove

REPOSITORY
  rKDEVEMBEDDED KDevelop Plugin: Embedded Platforms

REVISION DETAIL
  https://phabricator.kde.org/D1674

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: patrickelectric, brauch, tcanabrava
Cc: tcanabrava, patrickelectric, brauch, kdevelop-devel