Review Request 127749: First review of embedded plugin, development of embedded systems for KDevelop.

Wed Apr 27 16:35:33 UTC 2016

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127749/#review94910
-----------------------------------------------------------


Just a technical/code review -- will look over the functionality as soon as it is in a separate repo and I can build it :)


File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment555>

    ?


File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment556>

    The way you select whether you have a space behind a parenthesis or not is confusing ;)
    I'd suggest to either always or never have one (I prefer never)


File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment557>

    ?


File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment558>

    This function name is very confusing.


File Attachment: embedded.cpp - embedded.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment559>

    I don't think this action is frequently used enough to have a default shortcut ... how often do you use this? five times a year?


File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment560>

    coluns?


File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment561>

    I don't think you need this if you include e.g. QDialog -- I've never seen it anywhere


File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment562>

    this file is missing a copyright header (as are several others)


File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment563>

    please use uppercase names for classes
    Also, you inherit from Ui::arduinowindow and then you have a meber of that type as well? that seems wrong


File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment564>

    why don't you simply store the two vectors? that doesn't seem more difficult to use in any place than the pairs and saves a lot of code ...


File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment565>

    const &


File Attachment: arduinowindow.h - arduinowindow.h
<https://git.reviewboard.kde.org//r/127749/#fcomment566>

    Technically speaking, you have to call beginResetModel / endResetModel here.
    I'd also suggest to move this implementation to the .cpp file.


File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment567>

    Also just use some way of automatic memory management (e.g. QScopedPointer).


File Attachment: arduinowindow.cpp - arduinowindow.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment568>

    ?? :D
    also below, those includes seem a bit random, I don't think you need them


File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment569>

    Oh no please don't. Use KArchive.
    Network with QNMA is ok, but KIO::copy would probably be simpler there as well


File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment570>

    Why do we need an OS switch here? QFileDialog works the same on all those operating systems.


File Attachment: firsttimewizard.cpp - firsttimewizard.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment571>

    also here, why the #ifdef


File Attachment: toolkit.cpp - toolkit.cpp
<https://git.reviewboard.kde.org//r/127749/#fcomment572>

    put the path in a local variable


File Attachment: toolkit.h - toolkit.h
<https://git.reviewboard.kde.org//r/127749/#fcomment573>

    generic header guard names are always a source of infinite fun when somebody else uses the same one ;)


File Attachment: CMakeLists.txt - CMakeLists.txt
<https://git.reviewboard.kde.org//r/127749/#fcomment574>

    It makes sense to define those here. But then you should use configure_file to write them out, and also use that to check the version in the code


File Attachment: CMakeLists.txt - CMakeLists.txt
<https://git.reviewboard.kde.org//r/127749/#fcomment575>

    use configure_file instead


- Sven Brauch


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/20160427/31395745/attachment-0001.html>