Bring back memory view
ClosedPublic

Authored by volden on Jan 21 2017, 9:27 AM.

Details

Summary

Bring Memory view vidget back.

Make CMake search for OktetaCore and OktetaGui.

Change the widget to use Okteta ByteArrayModel and ByteArrayColumnView

Minor code cleanup including:

  • Converted to new signal and slot syntax
  • fix include guard
  • prepend membervars with m_
  • adapt to interface changes in kdevelop/debuggers
  • Use configure-time CMake for define
Test Plan

Manually testing done. Seems to work :-)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
volden updated this revision to Diff 10405.Jan 21 2017, 9:27 AM
volden retitled this revision from to Bring back memory view.
volden updated this object.
volden edited the test plan for this revision. (Show Details)
volden added a reviewer: KDevelop.
volden set the repository for this revision to R32 KDevelop.
volden added a project: KDevelop.
volden added a subscriber: kdevelop-devel.
volden updated this revision to Diff 10406.Jan 21 2017, 9:37 AM
volden removed R32 KDevelop as the repository for this revision.
volden changed the visibility from "volden (Morten Volden)" to "Public (No Login Required)".
volden updated this object.Jan 21 2017, 10:52 AM

Looks good from a first very quick look at Okteta related things. More later.

debuggers/gdb/memviewdlg.cpp
153

Now some more detailed look at the code. Code not tested so far.

debuggers/gdb/CMakeLists.txt
3

Only needed for expliciteness, OktetaGui pulls this in as required dep, so would fail itself if OktetaCore is not present.

71

Here use the imported target names OktetaCore and OktetaGui.
Again OktetaCore is only needed for expliciteness, will be pulled in by OktetaGui.

debuggers/gdb/memviewdlg.cpp
99–100

While touching the lines, nullptr for all pointers

243–251

This should be doable by simply resetting the data of the existing model, using ByteArrayModel::setData(), like (untested):

m_memViewModel->setData(reinterpret_cast<Okteta::Byte*>(m_memData), m_memAmount);

https://api.kde.org/4.x-api/kdesdk-apidocs/okteta/html/classOkteta_1_1ByteArrayModel.html#a44f6d09a054f80603978f7ce387085dc

Or did you try and meet a flaw with that?

volden updated this revision to Diff 10413.Jan 21 2017, 4:49 PM
volden set the repository for this revision to R32 KDevelop.

Addressed Friedrich's review comments.

Using the Okteta FullSizeLayoutStyle

mwolff requested changes to this revision.Jan 22 2017, 7:50 PM
mwolff added a reviewer: mwolff.
mwolff added a subscriber: mwolff.

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

debuggers/gdb/CMakeLists.txt
55

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

debuggers/gdb/memviewdlg.cpp
53

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

68

why did you remove the this parameter everywhere?

86

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

debuggers/gdb/memviewdlg.h
54

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

58

de-indent from here on below

64

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

This revision now requires changes to proceed.Jan 22 2017, 7:50 PM
volden updated this revision to Diff 10505.Jan 24 2017, 6:38 PM
volden updated this object.
volden edited edge metadata.
volden marked 6 inline comments as done.

Addressed Milian's review comments

volden added inline comments.Jan 25 2017, 2:29 PM
debuggers/gdb/memviewdlg.cpp
53

My Plan is this:

  1. Bring back memory view
  2. Add an autoupdate feature to the memview (either by default or an auto update button)
  3. Figure out an easier way to get the address of a specific variable transported to MemoryRangeSelector (Maybe rigth-click from the variables tool view, or something similar)

So I plan to do some more work on some of this code and I can definitely do this as well, but I think it probably belongs in a separate differential.

68

Good question. I started getting these in the log:

kdevelop(13251)/default unknown: QLayout: Attempting to add QLayout "" to KDevMI::GDB::MemoryRangeSelector "", which already has a layout

Which I gather are a result of

QGridLayout* gl = new QGridLayout(this);

and

QHBoxLayout* hb = new QHBoxLayout(this);

and the subsequent calls to addLayout() and the call to setLayout(l) . I will leave remove the this pointer from the two layouts and leave the rest as is.

86

Done. I assume you mean the other way around - right?

kossebau added inline comments.Jan 25 2017, 4:05 PM
debuggers/gdb/memviewdlg.cpp
68

Removing the this parameter is correct though for Qt5, when it comes to non-top-level layout instances. This seems a change vs. Qt4 where the this was only used for memory management, while now it is also used for defining the widget on which to use the layout as top-level layout. Which in the case of these two layouts would be not what is wanted, as also hinted by the warnings in the log.
Cmp. e.g. http://doc.qt.io/qt-5/qlayout.html#QLayout

So +1 from here for removing the this parameter :)

volden marked 4 inline comments as done.Jan 29 2017, 2:52 PM
mwolff requested changes to this revision.Jan 29 2017, 8:06 PM
mwolff edited edge metadata.

some more nitpicks (sorry!), but at least fixing the memory leaks is a must

debuggers/gdb/CMakeLists.txt
60

kdev-debuggers? config-gdb-plugin.h is more like it

debuggers/gdb/config-kdev-debuggers.h.cmake
1 ↗(On Diff #10505)

please still add a proper copyright header

3 ↗(On Diff #10505)

use #cmakedefine01

debuggers/gdb/memviewdlg.cpp
111–116

auto debugController = ...

114

style:

connect(debugController, &...,
        this, &...);
139–141

missing parent

236–237

remove this->

debuggers/gdb/memviewdlg.h
135

I know, old code, but this is leaked. I cannot find a dtor that deletes it at least. Use a proper container class, like QByteArray or {Q,std::}vector<char>

This revision now requires changes to proceed.Jan 29 2017, 8:06 PM
volden updated this revision to Diff 10726.Jan 30 2017, 3:06 PM
volden edited edge metadata.
volden marked 9 inline comments as done.

Fixed Milians review comments

mwolff accepted this revision.Jan 30 2017, 4:39 PM
mwolff edited edge metadata.

lgtm now, thanks a lot!

debuggers/gdb/memviewdlg.cpp
469–471

is this cast really required? I don't see why

This revision is now accepted and ready to land.Jan 30 2017, 4:39 PM
volden added inline comments.Jan 30 2017, 4:46 PM
debuggers/gdb/memviewdlg.cpp
469–471

Me neither. I'll remove this and then push.

This revision was automatically updated to reflect the committed changes.
kossebau reopened this revision.Jan 30 2017, 6:04 PM

Seems there was a hickup when pushing? :)

debuggers/gdb/config-gdb-plugin.h.cmake was removed from the commit which ended in the repo.

This revision is now accepted and ready to land.Jan 30 2017, 6:04 PM
mwolff closed this revision.Feb 2 2017, 9:08 AM

was submitted:

commit f5b33ab4330944b7fa38714e9410427fc2fcfdcb
Author: Morten Danielsen Volden <mvolden2@gmail.com>
Date: Mon Jan 30 21:39:11 2017 +0400

Bring memory view back
Summary:
Bring the memory view widget back into a compilable state.
Make CMake search for OktetaGui.
Change the widget to use Okteta ByteArrayModel and ByteArrayColumnView
Fix char* memleak - use QByteArray instead
Use configure-time CMake for define

Minor code cleanup including:
Converted to new signal and slot syntax
fix include guard
prepend membervars with m_
Adapt to interface changes in kdevelop/debuggers

Reviewers: #kdevelop, mwolff

Reviewed By: #kdevelop, kossebau, mwolff

Subscribers: kossebau, mwolff, kdevelop-devel

Differential Revision: https://phabricator.kde.org/D4232