Bug 327701

Summary: code import wizard: does not read folders
Product: [Applications] umbrello Reporter: arne anka <kde-bugs>
Component: generalAssignee: Joris Steyn <kde>
Status: RESOLVED INTENTIONAL    
Severity: normal CC: kde, ralf.habacker
Priority: NOR    
Version: 2.11.3   
Target Milestone: ---   
Platform: Debian unstable   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description arne anka 2013-11-16 20:35:35 UTC
code import wizard does not read folders, and thus does not work recursively. does not matter if i check "include subdirectories" or not.
i would have to manually select every java-class i want to have imported, and then there are two other bugs: 
- "select all" does not work, not even ctrl+a ("deselect all" however _does_ work), which means that together with a missing "expand all" feature the code import is unusable for all but very small projects
- file dialog opens with common order reversed (Z-A)
Comment 1 Ralf Habacker 2013-12-01 00:25:14 UTC
A debugging session shows that  QFileSystemModel::row() returns zero if the related dir is not expanded, see below: 

void CodeImpSelectPage::treeClicked(const QModelIndex& index)
{
....
    uDebug() << "item at row=" << index.row() << " / column=" << index.column();
    QFileSystemModel* indexModel = (QFileSystemModel*)index.model();
    QFileInfo fileInfo = indexModel->fileInfo(index);
    if (fileInfo.isDir()) {
        // QFileSystemModel returns zero childrens for a collapsed directory
        int rows = indexModel->rowCount(index);
...
The code import wizard has code to select and count file recursivly with selected  "include subdirs", but it fails because of the zero childs mentioned above.
                QFileInfo childInfo = indexModel->fileInfo(childIndex);
                if (childInfo.isDir() && ui_subdirCheckBox->isChecked()) {
                    treeClicked(childIndex);
                }

I found a partial workaround by expanding the directory before fetching the children. 
    if (fileInfo.isDir()) {
        // QFileSystemModel returns zero childrens for a collapsed directory
        ui_treeView->setExpanded(index, true);
        qApp->processEvents();
        int rows = indexModel->rowCount(index);

unfortunally a few tests show that this fix is not enough to set this bug as resolved.
Comment 2 Joris Steyn 2014-02-25 21:24:37 UTC
Git commit c2bba32636e3b7b8646dd01008f70efab8c86d02 by Joris Steyn.
Committed on 25/02/2014 at 18:06.
Pushed by jorissteyn into branch 'master'.

Sort files ascending by name in code import wizard

M  +4    -0    umbrello/codeimpwizard/codeimpselectpage.cpp

http://commits.kde.org/umbrello/c2bba32636e3b7b8646dd01008f70efab8c86d02
Comment 3 Joris Steyn 2014-02-27 21:21:30 UTC
QFileSystemModel does lazy loading of directory contents, so the checkbox only
works if the parent directory has been previously expanded by the user - which
is very confusing behaviour. The wizard does no real recursive scanning of
directories.

Loading and selecting all child items when clicking on a directory item is not
easy to implement: fetchMore() works asynchronously. So I tried to split up the
recursive/non-recursive selection in a way that allows selecting individual files
(current behaviour) as well as (what most users expect / need) just a single 
directory.

Ralph, I'll push this to work/327701. Could you check if this works alright for 
you? Maybe you've got a better idea?
Comment 4 Joris Steyn 2014-02-27 21:23:37 UTC
Git commit 7be6bf1bf86bea9ecc6e4ff4cfbe114c59e6aced by Joris Steyn.
Committed on 27/02/2014 at 20:19.
Pushed by jorissteyn into branch 'work/327701'.

Import wizard allows recursive import of directory

Selecting sparse files (old behaviour) is still possible by unchecking the
checkbox. The recursive behaviour seems like the most common use case so the
option is checked by default.

Changes in this commit:

* rename the 'include subdirectories' option to 'recursive directory import'
* when checked, the tree view shows only directories
* when checked, only a single item can be selected. The file counter counts all
files contained in the selected directory (recursively).
* the 'select all' button has been removed (didn't work, don't see a use case
for it)

M  +62   -106  umbrello/codeimpwizard/codeimpselectpage.cpp
M  +5    -3    umbrello/codeimpwizard/codeimpselectpage.h
M  +7    -11   umbrello/codeimpwizard/codeimpselectpage.ui

http://commits.kde.org/umbrello/7be6bf1bf86bea9ecc6e4ff4cfbe114c59e6aced
Comment 5 Ralf Habacker 2014-02-28 00:05:22 UTC
(In reply to comment #3)
> QFileSystemModel does lazy loading of directory contents, so the checkbox
> only works if the parent directory has been previously expanded by the user -
> which is very confusing behaviour. 
agree 
>The wizard does no real recursive scanning of directories.

> Loading and selecting all child items when clicking on a directory item  is
> not easy to implement: fetchMore() works asynchronously. So I tried to split up
> the recursive/non-recursive selection in a way that allows selecting individual
> files (current behaviour) as well as (what most users expect / need) just a single
> directory.
> 
> Ralph, I'll push this to work/327701. Could you check if this works alright
> for  you?
works but has issues see below. 

 Maybe you've got a better idea?
We should take a look at the related use cases. 
1. With bug 315417 for example the reporter requested to import a single or less number of files, which is suboptimal with the recent import wizard. Much faster is to use the logical View context menu entry  "import file(s)", which has been implemented on the request and because I personally also missed this feature. 
2. Importing a whole project (which is defined as "all subdirectories of a given dir") is also much faster by using the logical View context menu entry  "import project".
3. What is left over is a use case for selecting a mix of directories and files, for which the recent dialog is also suboptimal because of the little size and overall handling. The system provided file or directory selection dialogs are much better in this and other areas like recent path list, different views, net access, size handling and common ui style and I don't think that we like or should reimplement this in umbrello. I think it is always better to use system provided dialogs because of the native look and feel and to extend them if possible and required. 
I personal like the selected files number counting you have fixed, but how long does it take if someone select the root dir of a TB big network share ? I did this with my local 90GB big linux root dir; it let the wizard freeze for several minutes and I have to kill umbrello. :-(
Would it be possible to avoid any freezes especial when using network shares ? If no we should drop this completly.
Comment 6 arne anka 2016-07-13 11:28:02 UTC
stopped using it a long time ago