[Differential] [Requested Changes To] D4232: Bring back memory view

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a reviewer: mwolff.
mwolff added a comment.
This revision now requires changes to proceed.


  pretty good stuff, awesome work! I have some minor nitpicks. In the future, you could make reviewing easier by not mixing functionality patches with cleanup patches

INLINE COMMENTS

> CMakeLists.txt:49
> +if (OktetaGui_FOUND)
> +    add_definitions(-DWITH_MEMVIEW=1)
>      list(APPEND kdevgdb_SRCS

please use a header file that can be included on demand and gets created with cmake's configure_file

the advantage is that only the files that actually depend on the memview can then include that file and not everything gets the define set

> memviewdlg.cpp:53
>  {
> -public:
> -    QLineEdit* startAddressLineEdit;
> -    QLineEdit* amountLineEdit;
> -    QPushButton* okButton;
> -    QPushButton* cancelButton;
> -
> -    MemoryRangeSelector(QWidget* parent)
> -    : QWidget(parent)
> -    {
> -        QVBoxLayout* l = new QVBoxLayout(this);
> +    QVBoxLayout* l = new QVBoxLayout();
>  

consider using a .ui file generated with Qt designer instead as a follow-up cleanup

> memviewdlg.cpp:68
> -    {
> -        QVBoxLayout* l = new QVBoxLayout(this);
>  

why did you remove the this parameter everywhere?

> memviewdlg.cpp:86
>  
> +    connect(startAddressLineEdit, &QLineEdit::returnPressed, [=]() {
> +              okButton->animateClick();

please always use the 4-arg connect, i.e. pass a "this" before a lambda to make sure it gets properly unconnected when this gets deleted

also, prefer explicit capture lists over the catch-all ones: =/&

here, replace = with okButton

> memviewdlg.h:54
> +   */
> +    class MemoryRangeSelector : public QWidget
> +    {

all of this class is private, is that really your intention? I'd remove the friend below and keep it public like it used to be. also, moving it to the .cpp is better if at all possible, simply forward-declare it here

> memviewdlg.h:58
> +        MemoryRangeSelector(QWidget* parent);
> +        private:
> +            QLineEdit* startAddressLineEdit;

de-indent from here on below

> memviewdlg.h:64
> +
> +        friend class MemoryView;
> +    };

maybe make it all public if you only access it from the view?

REPOSITORY
  R32 KDevelop

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

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

To: volden, #kdevelop, mwolff
Cc: mwolff, kossebau, kdevelop-devel, Pilzschaf, akshaydeo, surgenight, arrowdodger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170122/f9ec41fc/attachment-0001.html>