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

[Arnold Dumas]

> On Nov. 1, 2013, 12:54 p.m., Milian Wolff wrote:
> > app/mainwindow.cpp, line 476
> > <http://git.reviewboard.kde.org/r/113523/diff/1/?file=206621#file206621line476>
> >
> >     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

DocumentWidget now inherits QStackedWidget. The first one is the display page and the second one is the loading page.


> On Nov. 1, 2013, 12:54 p.m., Milian Wolff wrote:
> > app/mainwindow.cpp, line 534
> > <http://git.reviewboard.kde.org/r/113523/diff/1/?file=206621#file206621line534>
> >
> >     this is what always should be done in openFile

Sorry I didn't get this one.


> On Nov. 1, 2013, 12:54 p.m., Milian Wolff wrote:
> > app/mainwindow.cpp, line 451
> > <http://git.reviewboard.kde.org/r/113523/diff/1/?file=206621#file206621line451>
> >
> >     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.

Durings the tests, It didn't causes any crash but yes, that could lead to tricky behaviors. It's fixed.


> On Nov. 1, 2013, 12:54 p.m., Milian Wolff wrote:
> > app/mainwindow.cpp, line 498
> > <http://git.reviewboard.kde.org/r/113523/diff/1/?file=206621#file206621line498>
> >
> >     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

What do you think of this ? Basically, each instance of DocumentWidget has its own ParseWorker. All the progress bar logic is done inside the DocumentWidget class. When all the diagrams and similar stuff is created, a signal is emitted. The MainWindow::parserFinished() slot will be called to do all the connections using the sender() method to recognize the DocumentWidget's instance that just finished loading. 

void MainWindow::openFile(const KUrl& file)
{
    Q_ASSERT(file.isValid());

    DocumentWidget* documentWidget = new DocumentWidget;

    for (int i = 0; i < ui.documents->count(); ++ i) {
        // If the file is already opened, then highlight it.
        if (qobject_cast<DocumentWidget*>(ui.documents->widget(i))->file() == file) {
            ui.documents->setCurrentIndex(i);
        }
    }

    m_changingSelections.insert(documentWidget, false);
    ui.documents->addTab(documentWidget, file.fileName());
    ui.documents->setCurrentIndex(ui.documents->count() - 1);

    // Consider the parserWorker is in the DocumentWidget class.
    documentWidget->open(file, m_allocatorModel);

    connect(documentWidget, SIGNAL(finished()),
        this, SLOT(parserFinished()));
}

void MainWindow::parserFinished()
{
    // All the connect stuff here
    // Using sender() method to get a pointer to the just loaded MassifDocument.
}

void DocumentWidget::openFile(const KUrl& file, QStringListModel* allocatorModel)
{
    parseWorker->parse(file, allocatorModel->stringList());
    // Connections to parserError and parserFinished slots.
}

void DocumentWidget::parserFinished()
{
   // Create the diagrams and all the stuff around.
}


> On Nov. 1, 2013, 12:54 p.m., Milian Wolff wrote:
> > app/mainwindow.cpp, line 737
> > <http://git.reviewboard.kde.org/r/113523/diff/1/?file=206621#file206621line737>
> >
> >     if no file is open, this should exit early

Fixed : 

void MainWindow::closeCurrentFile()
{
    if (!ui.documents->count()) {
        return;
    }

    // Former code
}


> On Nov. 1, 2013, 12:54 p.m., Milian Wolff wrote:
> > app/mainwindow.cpp, line 1167
> > <http://git.reviewboard.kde.org/r/113523/diff/1/?file=206621#file206621line1167>
> >
> >     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

I'm investigating ... For the moment, I've no clue about the origin of the problem. :(


- Arnold


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


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/c1daed3f/attachment.html>