Review Request 127749: First review of embedded plugin, development of embedded systems for KDevelop.
Tomaz Canabrava
tcanabrava at kde.org
Tue Apr 26 13:48:56 UTC 2016
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127749/#review94860
-----------------------------------------------------------
File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment524>
s/boardsId/boardIds
File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment525>
s/boardsName/boardNames
File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment526>
if you are using the boardIds just to get the list of the names of the boards, why don't you just use the Board::instance().boards ?
File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment527>
you used auto above, why not here too?
foreach(const auto& device, devices) { ... }
File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment528>
be consistent: you are using no brackets on some if's or foreaches (like the one above), and use on others. Please read the kdevelop coding style.
File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment529>
prefer to use QCDebug
File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment530>
why did you create the columns struct insteadof using a QStringList column_id; QStringList column_names? that way you don't need to copy each element on a QVector every time you need to populate thigns, just do column_names = names;
File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment531>
this 'getData' method is exactly the same as the data() method, with the exception that it returns a column:
QString Id = model.getData(modelIndex.row()).id;
QString Name = model.data(modelIndex).toString();
Also, an empty QString() is (), and not ("").
File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment532>
unused.
File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment533>
you are deleting a uncreated variable, = crash.
File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment534>
convert to new style signal.
File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment535>
convert to new style signal.
File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment536>
memory leak - you never free embeddedWindow.
File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment537>
memory leak
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment538>
->clear();
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment539>
->clear();
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment540>
initialize in the initializer list.
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment541>
Initialize in the initializer list, also, memleak - you never free mDownloadManager.
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment542>
use a random generated name.
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment543>
magic number: create a constant for it and add a comment on what this is.
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment544>
QStringList extractArgs;
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment545>
Don't call a extrenal program to extract a file, use KArchive.
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment546>
static initialize it:
static QStringList defaultArduinoPath = { strings here };
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment547>
caontins?
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment548>
return QString();
File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment549>
return QString();
File Attachment: firsttimewizard.h - firsttimewizard.h
<https://git.reviewboard.kde.org//r/127749/#fcomment550>
you don't use settings like this, they should be created on the stack, not on the heap.
- Tomaz Canabrava
On April 26, 2016, 3:05 a.m., patrick pereira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127749/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 3:05 a.m.)
>
>
> Review request for KDevelop, Sven Brauch, Kevin Funk, and Tomaz Canabrava.
>
>
> Description
> -------
>
> Embedded plugin for KDevelop
>
>
> Diffs
> -----
>
>
> Diff: https://git.reviewboard.kde.org/r/127749/diff/
>
>
> Testing
> -------
>
> Review of embedded plugin for KDevelop
> Plugin created to help the development of embedded systems.
>
> .
> |-- CMakeLists.txt
> |-- README.md
> |-- arduinowindow.cpp
> |-- arduinowindow.h
> |-- arduinowindow.ui
> |-- boardsimg
> | |-- ATmega644c3.svg
> | |-- ATtiny85_breadboard.svg
> | |-- Adafruit_Atmega32u4_Breakout.svg
> | |-- ArduPilotMega_v15_breadboard.svg
> | |-- Arduino\ Nano3_breadboard.svg
> | |-- Arduino\ Voice\ Shield-v16_breadboard.svg
> | |-- Arduino-BLE-LowPower-RevB-final_5_breadboard.svg
> | |-- Arduino-Ethernet-v11_breadboard.svg
> | |-- Arduino-Fio-v22_breadboard.svg
> | |-- Arduino-Mini-v5_breadboard.svg
> | |-- Arduino-Pro-Mini-v13_breadboard.svg
> | |-- ArduinoBluetoothMatev13_breadboard.svg
> | |-- Arduino_ADK_MEGA_2560-Rev3_breadboard.svg
> | |-- Arduino_DUE_V02b_breadboard.svg
> | |-- Arduino_Esplora_breadboard.svg
> | |-- Arduino_Ethernet_breadboard.svg
> | |-- Arduino_Leonardo_Rev3_breadboard.svg
> | |-- Arduino_MEGA_2560-Rev3_breadboard.svg
> | |-- Arduino_Micro_Rev03_breadboard.svg
> | |-- Arduino_Pro_breadboard.svg
> | |-- Arduino_bt07_breadboard.svg
> | |-- Ardumoto\ v13_breadboard.svg
> | |-- LICENSE.CC-BY-SA
> | |-- NetduinoPlus2.svg
> | |-- README.md
> | |-- TI_Launchpad_MSP430G2.1_breadboard.svg
> | |-- TI_Launchpad_MSP430G2_breadboard.svg
> | |-- Teensy_2.0_++_breadboard.svg
> | |-- Teensy_2.0_breadboard.svg
> | |-- Teensy_3.0+pads_breadboard.svg
> | |-- Teensy_3.0_breadboard.svg
> | |-- Udoo_dual_breadboard.svg
> | |-- Wiring_Sa99.svg
> | |-- arduino_Uno_Rev3_breadboard-old.svg
> | |-- arduino_Uno_Rev3_breadboard.svg
> | |-- arduino_Yun(rev1)_breadboard.svg
> | |-- arduino_mini_usb_adapter.svg
> | |-- arduino_uno(rev3)-icsp_breadboard.svg
> | |-- atlas-sientific_Conductivity\ Circuit_breadboard.svg
> | |-- atlas-sientific_D.O.\ Circuit_breadboard.svg
> | |-- intel-arduino-galileo_breadboard.svg
> | |-- intel-arduino-galileo_gen2_breadboard.svg
> | `-- intel_edison-hirosedf40_breadboard.svg
> |-- doc_templates
> | |-- CMakeLists.txt
> | |-- doxygen_cpp.txt
> | |-- phpdoc_php.txt
> | `-- rest_python.txt
> |-- embedded.cpp
> |-- embedded.h
> |-- embeddedproject
> | |-- %{APPNAMELC}.cpp
> | |-- %{APPNAMELC}.h
> | |-- %{PROJECTDIRNAME}.kdev4
> | |-- Makefile
> | |-- embeddedproject.kdevtemplate
> | |-- main.cpp
> | `-- options.kcfg
> |-- firsttimewizard.cpp
> |-- firsttimewizard.h
> |-- firsttimewizard.ui
> |-- kdevembedded.json
> |-- kdevembedded.qrc
> |-- kdevembedded.rc
> |-- patrick.kdev4
> |-- toolkit.cpp
> `-- toolkit.h
>
>
> File Attachments
> ----------------
>
> CMakeLists.txt
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/a8f69b53-9658-4552-8345-16a2f236fed8__CMakeLists.txt
> README.md
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/9d614ff1-c625-40b1-8a26-3ca3c6396a6f__README.md
> arduinowindow.cpp
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/6e95cc7f-a880-471f-8300-67c7584ef6fa__arduinowindow.cpp
> arduinowindow.h
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/29f98560-4081-4820-bc4b-cf0acba65f55__arduinowindow.h
> embedded.cpp
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/14591880-85df-45f7-b192-9059ef1a5a90__embedded.cpp
> embedded.h
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/3720a34d-40ff-45ad-853c-eb5955bc4287__embedded.h
> firsttimewizard.cpp
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/51f2f2ed-605c-49b1-87b7-85f6447c8274__firsttimewizard.cpp
> firsttimewizard.h
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/6171895d-b2ed-4f7d-85e8-c01dd511b17e__firsttimewizard.h
> kdevembedded.json
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/df4ccf03-fce7-43f5-be0e-11e00680ea12__kdevembedded.json
> kdevembedded.qrc
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/82f7d7ad-7e02-433c-aaa5-07fea7a8f66e__kdevembedded.qrc
> kdevembedded.rc
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/8f3d5a6d-bb86-48c9-84e7-cd5affcd750b__kdevembedded.rc
> toolkit.cpp
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/eb8ac57a-2b8c-4ba1-93d5-45e6be9d56f2__toolkit.cpp
> toolkit.h
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/88fa0dc5-af42-418a-84f8-6c1d35502c68__toolkit.h
> README.md
> https://git.reviewboard.kde.org/media/uploaded/files/2016/04/26/8d22467a-6f78-4c38-92af-c08438c2db13__README.md
>
>
> Thanks,
>
> patrick pereira
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160426/030d768f/attachment-0001.html>
More information about the KDevelop-devel
mailing list