Review Request 122197: Fixes to .ods import

[Jarosław Staniek]

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


Much better!


kexi/migration/AlterSchemaWidget.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52219>

    Some lesson regarding model handling:
    
    Possible crash:
        #5  0x00007f29ef117fd4 in QAbstractItemModelPrivate::removePersistentIndexData(QPersistentModelIndexData*) () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
        #6  0x00007f29ef118369 in ?? () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
        #7  0x00007f29ef1183af in QPersistentModelIndex::~QPersistentModelIndex() () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
        #8  0x00007f29efb4c0de in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
        #9  0x00007f29efb83ea8 in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
        #10 0x00007f29ef13b735 in QObject::~QObject() () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
        #11 0x00007f29ef69fdcc in QWidget::~QWidget() () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
        #12 0x00007f29efb76f39 in QTableView::~QTableView() () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
        #13 0x00007f29ef139168 in QObjectPrivate::deleteChildren() () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
        #14 0x00007f29ef69fd37 in QWidget::~QWidget() () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
        #15 0x00007f29eda5022b in KexiMigration::AlterSchemaWidget::~AlterSchemaWidget (this=0x14e1e40, __in_chrg=<optimized out>) AlterSchemaWidget.cpp:83
    
    Why? The table view accesses the model after it's deleted. 
    
    How to solve this? Use m_model = new AlterSchemaTableModel(m_table), then the code in destructor can be empty.



kexi/migration/importtablewizard.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52211>

    Instead of passing the whole 'args' please add



kexi/migration/importtablewizard.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52212>

    What if the checkbox is OFF? Old value stays in args.
    
    We need to always consider all cases :)



kexi/migration/importtablewizard.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52214>

    project -> table :)



kexi/migration/importtablewizard.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52213>

    This is local information, needed only in ariveAlterTablePage. It's common temptation to add plenty of global attributes but since that blurs the readability, I propose:
    
    It's enough to change arriveAlterTablePage() to arriveAlterTablePage(KPageWidgetItem *prevPage) and check what's the prevPage.



kexi/migration/importtablewizard.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52215>

    Above all: do we need to setTableSchema() when we're back from the next page?



kexi/migration/importtablewizard.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52216>

    Yes, if no data found we don't need to display the page that looks strangely: http://i.imgur.com/6Uu7221.png
    
    Instead let's display a KMessageBox::information() message "No data has been found in table <resource>%1</resource>. Select different table or cancel importing."
    and stay on the same page as before.



kexi/migration/importtablewizard.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52218>

    Just noticed that will break under right-to-left interfaces like Arabic.
    
    Let's change message to simpler/improved:
    
    txt = i18n("@info Table import wizard, final message", "<para>All required information has now been gathered. "
                   "Click <interface>Next</interface> button to start importing table <resource>%1</resource>.</para>"
                   "<note>Depending on size of the table this may take some time.</note>", m_alterSchemaWidget->nameWidget()->nameText());
                   
    So ln 459 is not needed



kexi/main/KexiMainWindow.cpp
<https://git.reviewboard.kde.org/r/122197/#comment52210>

    1. Why to pass b to as openingCancelled? It's unrelated.
    2. We want to open only when destinationTableName isn't empty exactly like KexiMainWindow::showProjectMigrationWizard() does.


- Jarosław Staniek


On Feb. 5, 2015, 1:10 p.m., Roman Shtemberko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122197/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2015, 1:10 p.m.)
> 
> 
> Review request for Calligra, Adam Pigg, Jarosław Staniek, Radosław Wicik, and Wojciech Kosowicz.
> 
> 
> Bugs: 336815
>     http://bugs.kde.org/show_bug.cgi?id=336815
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> Fix 336815 (path issue)
> Also have fixed with this patch:
> Check for an empty name.
> Names are no more converted to the lower case (there is some conflicts with this behavior, imported tables can not be opened after this (only after restarting an app (!?)))
> Check if name is not starting with a digit (conflicts with '_' at the begining as well).
> Default name is added based on sheet selected.
> 
> 
> Diffs
> -----
> 
>   kexi/main/KexiMainWindow.cpp f499951 
>   kexi/migration/AlterSchemaTableModel.h 2090b35 
>   kexi/migration/AlterSchemaTableModel.cpp 86ef62e 
>   kexi/migration/AlterSchemaWidget.h b29e7f9 
>   kexi/migration/AlterSchemaWidget.cpp ea7fedd 
>   kexi/migration/importtablewizard.h a0b4dca 
>   kexi/migration/importtablewizard.cpp f3d02b9 
>   kexi/plugins/migration/keximigrationpart.cpp 03c674e 
>   kexi/widget/KexiConnectionSelectorWidget.cpp 48d3f7e 
> 
> Diff: https://git.reviewboard.kde.org/r/122197/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Roman Shtemberko
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20150206/2cbb3478/attachment.htm>