Bug 358655 - Umbrello does not import private class
Summary: Umbrello does not import private class
Status: RESOLVED WORKSFORME
Alias: None
Product: umbrello
Classification: Applications
Component: general (show other bugs)
Version: 2.14.2 (KDE Applications 4.14.2)
Platform: Mint (Ubuntu based) Linux
: NOR normal
Target Milestone: ---
Assignee: Umbrello Development Group
URL:
Keywords:
Depends on:
Blocks: 363891
  Show dependency treegraph
 
Reported: 2016-01-27 20:54 UTC by Ken Standard
Modified: 2016-06-03 11:14 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
complete testcase (87 bytes, text/x-chdr)
2016-01-28 07:02 UTC, Ralf Habacker
Details
xmi file with imported header (generated with version 2.18.1) (10.27 KB, text/x-xmi)
2016-01-28 07:04 UTC, Ralf Habacker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ken Standard 2016-01-27 20:54:53 UTC
In a Qt 5.5.1 project, a private class is defined. It is referenced as a data type for a private variable pointer:

myheader.hpp
--------------------
private:
    class MyPrivateClass;
    MyPrivateClass *d_data;

myimplementation.cpp
--------------------------------
class MyImplementation::MyPrivateClass
{
. . .
}

only the *d_data variable from the header is imported and not the private class from the .cpp file.
Comment 1 Ralf Habacker 2016-01-28 07:02:54 UTC
Created attachment 96879 [details]
complete testcase
Comment 2 Ralf Habacker 2016-01-28 07:04:16 UTC
Created attachment 96880 [details]
xmi file with imported header (generated with version 2.18.1)
Comment 3 Ralf Habacker 2016-01-28 07:05:11 UTC
(In reply to Ralf Habacker from comment #2)
> Created attachment 96880 [details]
> xmi file with imported header (generated with version 2.18.1)
This file shows that at least with 2.18.1 the private class has been imported
Comment 4 Ken Standard 2016-01-28 14:42:10 UTC
Not seeing it here. 

In fact the whole source import process is very rough and is not as intuitive as one would expect.

I did not mention this before, to keep the issues separate. But whether you are importing a project or using the import wizard, many things are not imported. I only mentioned this one.

Your super simple case may work for you, but a much more complicated multi-folder project is a different story altogether in my experience. The only case I have not tried is the header by header import (a giant pain in a large project) from the wizard.
Comment 5 Ken Standard 2016-01-28 15:25:35 UTC
I have discovered an interesting phenomenon that affects this situation. 

If the .pro file contains either comments (lines with #)  or an include(include.file) there is a log message that treats it as an error.

Also, files are not displayed in the files list of the wizard - only folders - unless the list of filter patterns is modified.

Clearing the include line and comments in the .pro and selecting the header files after modifying the filter patterns, the headers will import correctly.

Very strange behavior.
Comment 6 Ralf Habacker 2016-01-28 15:39:19 UTC
(In reply to Ken Standard from comment #4)
> I did not mention this before, to keep the issues separate. But whether you
> are importing a project or using the import wizard, many things are not
> imported. I only mentioned this one.
You need to be more specific what 'many things are not imported' does mean because they may have different reasons. 

1. In the test case the private class is a forward declaration. Umbrello will only be able to create an empty class for MyPrivateClass. 

2. importing the following fragment
 class MyClass {
public:
    MyClass(const XYZ::AType &aparam);
}; 
gives umbrello no information, what XYZ is - it could be a namespace or a class. In this case umbrello imports 'XYZ' as class with a stereotype 'class-or-package', which could be reassigned in the tree view.

3. Imported code may have some #include ... statements, which may not in the default search path of umbrello, which is on unix

void CppImport::initialize()
... 
    ms_driver->addIncludePath(QLatin1String("/usr/include"));
    ms_driver->addIncludePath(QLatin1String("/usr/include/c++"));
    ms_driver->addIncludePath(QLatin1String("/usr/include/g++"));
    ms_driver->addIncludePath(QLatin1String("/usr/local/include"));
    const QStringList incPathList = Import_Utils::includePathList();

From the call to Import_Utils::includePathList()  a list of additional include directories can be specified on the command line with the environment variable UMBRELLO_INCPATH (path separator is ':' on unix/linux)  before starting umbrello.

You are refering to different cases ?
Comment 7 Ralf Habacker 2016-01-28 16:39:15 UTC
(In reply to Ralf Habacker from comment #6)
> You are refering to different cases ?
To be able to get a better idea: After importing you will find a dock window named 'protocol' which lists all errors and warnings from the import. Double clicking on entries in this window shows the related imported source file.
Comment 8 Ken Standard 2016-01-28 17:17:13 UTC
OK, I will be "more specific".

Your #1: This will always be the case with PIMPL coding style. As I mentioned; 
    after removing the comments (lines with #) from the .pro 
    after removing the include(myfile.pri) line from the .pro
    after modifying the filter patterns in the import Wizard
    after selecting specific files in the files list
the private classes were imported properly appearing as private classes in the logical view list of classes and with the forward declared items as a privately declared class in the imported class.

Your #3: I made no mention of any other file having an include except for the .pro. In that context I mentioned it reported as an error in the syntax. This is not a proper response by the import wizard since it is completely valid for Qt projects. Not only that, the # mark in the .pro file is also treated as an error anywhere it appears. This also is not valid since it is the designated comment mark for Qt project files. If a C++ comment ("//") appears in the code it too is not ignored as it should be, but treated as a code error as well being reported in the wizard import log as a syntax error. Interestingly, a "C" style comment (/* */) does not render an error and IS ignored.

Comment 7: I find no dock window, list, or other window, named or titled "Protocol". The import wizard has a "Logger" window and the main window has a "Log" dock window. There is also no menu choice to view any "Protocol" information.
Comment 9 Ralf Habacker 2016-01-29 10:43:06 UTC
(In reply to Ken Standard from comment #8)

> Comment 7: I find no dock window, list, or other window, named or titled
> "Protocol". The import wizard has a "Logger" window and the main window has
> a "Log" dock window.  
Sorry for confusion. "Protokoll" is the german translation of 'log'. I'm refering to the "log" window. The log window is filled with errors from import regardless started an import from the import wizard or using menu entry 'code'->'import project'
Comment 10 Ralf Habacker 2016-01-29 11:01:46 UTC
(In reply to Ken Standard from comment #5)
> I have discovered an interesting phenomenon that affects this situation. 
On umbrello start c++ is predefined as standard and opening the import wizard shows file extensions supported by the cxx support.

> If the .pro file contains either comments (lines with #)  or an
> include(include.file) there is a log message that treats it as an error.
.pro files are not intended to be imported by the c++ import.

> Also, files are not displayed in the files list of the wizard - only folders
> - unless the list of filter patterns is modified.
You cleaned up the list of file patterns  and entered *.pro as extension to search for ? 
As said .pro is not a supported file extension for c++ import.
Comment 11 Ralf Habacker 2016-01-29 11:23:27 UTC
(In reply to Ken Standard from comment #4)
> Not seeing it here. 
> 
> In fact the whole source import process is very rough and is not as intuitive as one would expect.
What could be simplier as 
1. Start umbrello
2. setup required language with  'Code'->Active Language' (defaults to c++)
3. click on 'Code'->'Import project', select directory and wait until import is finished.
Comment 12 Ken Standard 2016-01-29 14:19:57 UTC
OK, maybe we are having a language barrier - not sure.

When talking about "project import" for a Qt project, you cannot ignore the .pro file - it is the project. Are we redefining "project" to mean only the .h,.hpp,/h++, etc. and the .cpp implementations in the "project"?

Or are we accepting the "project" definition as is common to Qt and defined by the .pro file.

Besides all that. When the actions described are performed and the entire project is imported correctly.

It serves no purpose for me to post a bug if I have not performed every options to resolve the problem BEFORE submitting the bug. That should be obvious from comment 8. My point is that I should not have to do half of that. The project files should alreays be displayed according to the filter pattern, instead it must be modified first, then they are displayed. 

Secondly, folders are included in the selection process but that does not select the files underneath that match the selection pattern - not intuitive. Do one or the other but not both.

Even when all the files in a folder are selected, the import to a class diagram is not always 100%. Sometimes files are not converted. I gave the example of a .cpp file where a "//" comment was detected as an error. This syntax is completely valid C++ comment style and should not error. When coverted to a "C" style comment (/* */) there is no error. This is not proper - both are correct.

I submitted the bug because your procedure in comment 11 does not work - especially if headers and implementation files are in separate folders or if there are multiple sub-projects.

I don't want to get too far afield here but my original bug is still valid and I think I have proven it.
Comment 13 Ralf Habacker 2016-01-29 20:29:57 UTC
Git commit 67a97004fc8353544326d4875e3c236813a1bb6a by Ralf Habacker.
Committed on 29/01/2016 at 20:30.
Pushed by habacker into branch 'Applications/15.12'.

Add c++ import testcases for parsing comments.

A  +16   -0    test/import/cxx/comments-class-members.h     [License: UNKNOWN]  *
A  +16   -0    test/import/cxx/comments-class-methods.h     [License: UNKNOWN]  *
A  +12   -0    test/import/cxx/comments-class.h     [License: UNKNOWN]  *
A  +7    -0    test/import/cxx/comments-file-variant1.h     [License: UNKNOWN]  *
A  +7    -0    test/import/cxx/comments-file-variant2.h     [License: UNKNOWN]  *
A  +3    -0    test/import/cxx/comments-file-variant3.h     [License: UNKNOWN]  *
A  +6    -0    test/import/cxx/comments.h     [License: UNKNOWN]  *

The files marked with a * at the end have a non valid license. Please read: http://techbase.kde.org/Policies/Licensing_Policy and use the headers which are listed at that page.


http://commits.kde.org/umbrello/67a97004fc8353544326d4875e3c236813a1bb6a
Comment 14 Ralf Habacker 2016-01-29 23:04:53 UTC
(In reply to Ken Standard from comment #12)
> OK, maybe we are having a language barrier - not sure.
> 
> When talking about "project import" for a Qt project, you cannot ignore the
> .pro file - it is the project. Are we redefining "project" to mean only the
> .h,.hpp,/h++, etc. and the .cpp implementations in the "project"?
> 
> Or are we accepting the "project" definition as is common to Qt and defined
> by the .pro file.
Okay I'm on track:  this bug is a feature request and  should have be  named  more like 'umbrello should support importing a complete Qt project' by pointing umbrello to the .pro file, which should then be read in and parsed to get a list header and source file to be imported.
 
> 
> Besides all that. When the actions described are performed and the entire
> project is imported correctly.
> 
> It serves no purpose for me to post a bug if I have not performed every
> options to resolve the problem BEFORE submitting the bug. That should be
> obvious from comment 8. My point is that I should not have to do half of
> that. The project files should alreays be displayed according to the filter
> pattern, instead it must be modified first, then they are displayed. 
I see, the term 'Import project' may be somehow irritating, it should be renamed to 'import from directory' or similar.
 
> Secondly, folders are included in the selection process but that does not
> select the files underneath that match the selection pattern - not
> intuitive. Do one or the other but not both.
looks like that the import wizard is broken and need to be fixed.

> Even when all the files in a folder are selected, the import to a class
> diagram is not always 100%. Sometimes files are not converted. I gave the
> example of a .cpp file where a "//" comment was detected as an error. 
But you did not provide a dedicated test case, so it's impossible to fix :-(

> syntax is completely valid C++ comment style and should not error. When
> coverted to a "C" style comment (/* */) there is no error. This is not
> proper - both are correct.
To be able to see what umbrello supports and what not I added some c++ import comment  testcases, see comment 13. 
If your problem is not covered please give an real world example. 
> 
> I submitted the bug because your procedure in comment 11 does not work -
> especially if headers and implementation files are in separate folders or if
> there are multiple sub-projects.
Because umbrello currently do not support importing a .pro file directly, there are at least two workarounds available:
1.  Use  'Code'->Import Project several time for each related directory. 
2.  umbrello supports a command line option --import-files <files> to which you can give a list of files to import
    The list may be generated from a command line tool parsing the .pro file.
Comment 15 Ralf Habacker 2016-01-29 23:13:56 UTC
Git commit dd45e245990253af19988268c3a599cad91d8719 by Ralf Habacker.
Committed on 29/01/2016 at 23:14.
Pushed by habacker into branch 'Applications/15.12'.

Add more c++ import testcases for parsing comments.

A  +16   -0    test/import/cxx/comments-class-enums.h     [License: UNKNOWN]  *
A  +16   -0    test/import/cxx/comments-class-typedefs.h     [License: UNKNOWN]  *
M  +2    -0    test/import/cxx/comments.h

The files marked with a * at the end have a non valid license. Please read: http://techbase.kde.org/Policies/Licensing_Policy and use the headers which are listed at that page.


http://commits.kde.org/umbrello/dd45e245990253af19988268c3a599cad91d8719
Comment 16 Ken Standard 2016-01-30 13:17:08 UTC
I am kind of taken back by some of your comments.

Why would you need a test case for valid, commonly used, generic GNU C++ syntax? It seems unnecessary,  redundant, and easily recreated.

I see that umbrello is not as Qt aware as I had first believed. Also strange, since KDevelop, Qt SDK Framework, and even Mono use project descriptor files. This should not have been a surprise.

My notes on the comment 8 post here was not, as you describe, "irritation", rather fascination that such a simplistic approach is taken to what should be intuitive. And, yes, a more descriptive choice would have been clearer.

I think there is also still a question for projects where headers are in separate folders from source code and where headers and code may be grouped in folders rather than in a flat directory structure with all files together. Maybe a "recursive" option on the file selection?





Yes, the import wizard is definitely broken. I discovered the workaround you describe just after my bug post. Maybe now we can change the status from "unconfirmed" to "confirmed" "working" or something?
Comment 17 Ralf Habacker 2016-06-03 11:13:15 UTC
(In reply to Ken Standard from comment #16)
> Why would you need a test case for valid, commonly used, generic GNU C++
> syntax? It seems unnecessary,  redundant, and easily recreated.
To be able to verify that this is working in the future too. Someone working on related parts can simply import given testcases  to see if something is broken.

> I see that umbrello is not as Qt aware as I had first believed. 
Please file a feature request mentioned in comment 14 or provide a patch. I'm happy to review :-)

> Also strange, since KDevelop, Qt SDK Framework, and even Mono use project
> descriptor files. This should not have been a surprise.
What is the benefit of having a dedicated project descriptor file ? If you are missing some features which would only be possible with project descriptors, feel free to submit a feature request. 

> My notes on the comment 8 post here was not, as you describe, "irritation",
> rather fascination that such a simplistic approach is taken to what should
> be intuitive. And, yes, a more descriptive choice would have been clearer.
fixed, see bug 363891

> I think there is also still a question for projects where headers are in
> separate folders from source code and where headers and code may be grouped
> in folders rather than in a flat directory structure with all files
> together. Maybe a "recursive" option on the file selection?