[MassifVisualizer] Review Request 113523: Implementation of multiple document interface

Fri Nov 1 12:54:46 UTC 2013

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113523/#review42770
-----------------------------------------------------------


app/mainwindow.h
<http://git.reviewboard.kde.org/r/113523/#comment30917>

    please rename to documentwidget, eventually I want to make this tool work with other memory profilers and thus hardcoding the massif name in classes will become confusing eventually.


app/mainwindow.h
<http://git.reviewboard.kde.org/r/113523/#comment30916>

    just documentChanged


app/mainwindow.h
<http://git.reviewboard.kde.org/r/113523/#comment30918>

    currentDocument


app/mainwindow.h
<http://git.reviewboard.kde.org/r/113523/#comment30932>

    I'd make it a QHash<DocumentWidget*, bool>


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30931>

    this is now wrong, remove it


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30919>

    this looks wrong. either remove the dead code or fix it properly.
    
    I assume you simply couldn't test b/c kgraphviewer does not work for you?
    
    generally, I think this block should be moved as-is to the new documentwidget. Note: I mean the full conditional block, i.e. UI cleanup etc. pp.


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30920>

    rename to ui.documents


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30921>

    note that the explicit KUrl copying must stay in as far as I see. Otherwise you might end up with strange aliasing bugs (the "madness" the comment hints at). I.e. you pass in a const& to the file url, then closeFile is called eventually, setting the url to KUrl(), later you try to load the file url but it is now a KUrl() since it aliased the write.


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30923>

    the progress bar should be part of the documentwidget, after all, multiple files could be loaded at the same time (eventually) with distinct progresses.
    
    as I say below, here you should a) create the document widget, and b) eventually hook up the parse job to the progress signals. without threadweaver, you'll have to do the progress mapping manually


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30922>

    this should be created (if not existing already) in the openFile function.
    
    in this function, you only need to do the manual mapping of input url -> documentwidget -> setdata.
    
    note that this should become obsolete as soon as you add the threadweaver and per-file parse jobs. then this could easily be done via a signal/slot direction.
    
    as such, the debug output above should also go to the documentwidget's setData 


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30924>

    removing a widget should only be required when you close a file


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30925>

    this is what always should be done in openFile


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30926>

    should only be done if the document is currently selected, i.e. assume this:
    
    you open a file, it takes long to parse and you see the progress bar. you go back to another document and once the long loading file finished, you don't suddenly want to set the model of the tree view.
    
    probably gets fixed anyways by moving this block to openFile


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30927>

    if no file is open, this should exit early


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30928>

    just documentChanged


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30929>

    then please fix it :)
    
    also, this will connect multiple times, if you switch between files and then you get multiple treeSelectionChanged invocations for a single signal


app/mainwindow.cpp
<http://git.reviewboard.kde.org/r/113523/#comment30930>

    make const


app/massifdocumentwidget.h
<http://git.reviewboard.kde.org/r/113523/#comment30934>

    here and in the .cpp, please also keep the line with my copyright, considering you didn't change the actual implementation too much, but just moved code around :)
    
    which reminds me, that you definitely should add your copyright also to the mainwindow.{h,cpp} files!


app/massifdocumentwidget.h
<http://git.reviewboard.kde.org/r/113523/#comment30933>

    here and below: make the getters const


- Milian Wolff


On Oct. 31, 2013, 5:23 p.m., Arnold Dumas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113523/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 5:23 p.m.)
> 
> 
> Review request for Massif Visualizer.
> 
> 
> Repository: massif-visualizer
> 
> 
> Description
> -------
> 
> All the former logic of creating the charts, the legend ... etc is now contained in the MassifDocumentWidget.
> This widget is instanciated by the top-most tab widget. When the active tab changes, the massifDocumentChanged slot is called.
> Some actions are enabled/disabled regarding of the situation. I noticed no particular bug or misbehavior when testing.
> One more thing needs to be done: parallel file loading using ThreadWeaver.
> 
> 
> Diffs
> -----
> 
>   app/CMakeLists.txt 31290f6 
>   app/main.cpp be938b4 
>   app/mainwindow.h 2acc6a8 
>   app/mainwindow.cpp b33ff60 
>   app/mainwindow.ui 8ac4119 
>   app/massifdocumentwidget.h PRE-CREATION 
>   app/massifdocumentwidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/113523/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arnold Dumas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/massif-visualizer/attachments/20131101/6631b1c0/attachment-0001.html>