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

On October 5th, 2012, 8:27 p.m., Michel Ludwig wrote:

Ok, this looks very good already. I just have have a few suggestions for improving the patch even more.

Sorry, but I don't see how 'cleanStruct()' could help avoid any race conditions. Could you elaborate on that?

On October 5th, 2012, 10:17 p.m., Eugene Shalygin wrote:

As well as I remember, I came to understanding while I was analyzing why do I see absence of the master documents when I have many projects openend. What happened is that same info object has been scheduled for parcing twice or more. I do not remember, unfortunately, what was leading to that. Some of the projects are sharing the same files, if that can play role. Anyway, in that case the second scheduling cleans up structure that was built in result of the first parsing, and this cleaned object is used for project tree building or something like that. And I saw incorrect project trees, empty bibliography completion lists and so on. Then I moved cleaning into the install* methods, and that, as I thought, should lead to cleaning in the same thread that builds project tree, and the problem disappeared. And I thought that it is more logical to clean structure right before populating with the new one. So I would leave it even if double parsing will not (and shall not) happen.

On October 6th, 2012, 8:37 a.m., Michel Ludwig wrote:

Hhmmm, I think if this problem still occurs it must have been caused by something else as all the code in TextInfo and the structure view is executed in the GUI thread, which means that no race conditions can occur (the output parser is passed to the event loop of the GUI thread using a blocking signal/slot connection, see 'KileDocument::Manager::handleParsingComplete'

So, at the places where 'cleanStruct' was called it had no effect as it was followed immediately by 'operator=' and everything GUI-related runs in the GUI thread only. 

On October 6th, 2012, 1:31 p.m., Eugene Shalygin wrote:

I disagree. When we call m_ki->structureWidget()->update(itemInfo, true) from projectOpenItem(), it was cleaning structure  immediately, and then was scheduling the parsing of the item. And if parsing  queue contains many items, result of parsing of some other item uses clean structure of this item. And this happens if the same items is scheduled for parcing twice.

On October 6th, 2012, 3:46 p.m., Michel Ludwig wrote:

Ok, but that's a different issue. Your fix was right for the reason that you had emptied 'Info::cleanStruct()'.
Pardon me, but cleanStruct() is what I've added. Before this code was in updateStruct() and causing above described problem. So, despite I did not understand your last comment completely, I think that main iea was that we leave clearStruct()?

On October 5th, 2012, 8:27 p.m., Michel Ludwig wrote:

src/documentinfo.cpp (Diff revision 4)
const long* TextInfo::getStatistics(KTextEditor::View *view)
542
		return KUrl();
551
		return Info::url();
I don't know how much work this will be but could you try to remove the 'Info::url()' method completely? It just causes too many problems, for example when files are renamed.

It should be sufficient to use 'urlFor' from the doc manager instead.

On October 5th, 2012, 10:17 p.m., Eugene Shalygin wrote:

Well, I've tried. Indeed, it leads to many changes. Some of them are trivial, some are not. For example, Info::lastModifiedFile() uses url(). Where can I get doc manger from in that function?

Perhaps, it would be better if I remove introduced by patch changes in Info constructors, and switch to the using of doc manager in the parser thread, then you accept this patch ;) and further clean up can be done independently?

On October 6th, 2012, 8:36 a.m., Michel Ludwig wrote:

Ok, let's do it this way.

On October 6th, 2012, 1:31 p.m., Eugene Shalygin wrote:

Ups... Manager::urlFor(TextInfo* textInfo) calls Manager::itemFor(Info *docinfo, KileProject *project /*=0*/) which in turn calls Manager::itemFor(const KUrl &url, KileProject *project /*=0L*/) with docinfo->url()

Thus, by removing url from Info we destroy this chain.

On October 6th, 2012, 3:46 p.m., Michel Ludwig wrote:

Ok, let's wait with this for now then. The whole thing is a mess anyway and needs to be cleaned up!

Could you just remove the additional URL parameter that you have added from the patch?
Sorry, I can not :) If the parser does not know the url, it can not deliver the results to a proper destination. That is why I've started all this work with adding url to Info objects, because without that, the url exists only for Infos with documents. So the idea was to provide information to parser without loading KateDocument. This includes url and file contents.

On October 5th, 2012, 8:27 p.m., Michel Ludwig wrote:

src/kiledocmanager.cpp (Diff revision 4)
void Manager::projectOpenItem(KileProjectItem *item, bool openProjectItemViews)
1549
	//oops, doc apparently was open while the project settings wants it closed, don't trash it, update openState instead
It looks like this is still needed somewhere...

On October 5th, 2012, 10:17 p.m., Eugene Shalygin wrote:

Can not understand how that is possible: we create view now only if item->isOpen() == true, then why should we check it again? The same applies to the trashDoc() call, doesn't it?
Any comment about this?

On October 5th, 2012, 8:27 p.m., Michel Ludwig wrote:

src/kiledocmanager.cpp (Diff revision 4)
QStringList Manager::loadFileContents(const KUrl& url, const QString& encoding)
2559
			KIO::Job* getJob = KIO::file_copy(url, KUrl(tmpFile.fileName()), -1, KIO::Overwrite | KIO::HideProgressInfo);
Could you use 'KIO::NetAccess::download' here? And maybe do everything in one method?

It should make the code easier to understand.

On October 5th, 2012, 10:17 p.m., Eugene Shalygin wrote:

KIO::NetAccess::download() will show that fancy progress windows, right? Because it does not set KIO::HideProgressInfo flag, as I understand. If so, I would be the first one who ask for to remove these windows :)
You know, now I doub't that it is needed at all: what project item we need to access remotely via KIO being unable to pass it to latex? Just for the case when one opens project only for viewing, not for compiling...

On October 6th, 2012, 8:36 a.m., Michel Ludwig wrote:

Hehe, but in KDE4 they will only be shown in the notification widget, leaving the user in peace, and you can turn them off there ;)

I know that Kile cannot really pass remote documents to LaTeX at the moment but that's a bug, not feature. And besides, using NetAccess::Download only requires 2 lines of code ;)

On October 6th, 2012, 1:31 p.m., Eugene Shalygin wrote:

Well, I do not know, how to turn off these annoying messages only for particular "context", like in case of Kile - only for project items loading. Is it possible to turn them off for a particular application? And without checks for errors, explicit use of Job requires 4 lines :) NetAcces::download requires 3 lines at least, cause we nee to either use KTemporaryFile or to delete file by ourself.
In any case, since Kile refuses to load non-local project, and it is impossible to add project item which is outside of the project directory (BTW, why? It would be so nice to add bibliography databases directly, without creating symlinks in project directory... I see problems only for project archiving. Are there any more?) this feature will wait for its time.

On October 6th, 2012, 3:46 p.m., Michel Ludwig wrote:

QString tmpFile;
if(KIO::NetAccess::download(url, tmpFile, window)) {
loadFile(tmpFile);
KIO::NetAccess::removeTempFile(tmpFile);
}

and after that we can think about proper error handling, which hasn't been done at all so far.

If you don't want to change it, let me know and I will do it...
No problems! Of course I will change the patch. Anyway I'm not going to use that feature and will not see these popup window :))

- Eugene


On October 5th, 2012, 5:33 p.m., Eugene Shalygin wrote:

Review request for Kile.
By Eugene Shalygin.

Updated Oct. 5, 2012, 5:33 p.m.

Description

Kile shedules parsing of all projet items when it opens a project. But because Manager::m_projects does not contain currently loading project at that time, Manager can not resolve projet items to documents and parsing does not happen.

This patch adds curently parsing project to m_projects collection before invoking projectOpenItem() and thus parsing goes. Then we crate Info* object for each project item (if it does not exist) and sneding it to parsing.
In the next step, in Manager::handleParsingComplete(), TextInfo object is determined via opened documents list, as I understood. To install results for all project items, the patch adds a second branch that determines TextInfo object via KileProjectItem::getInfo() and the item is accessed via Manager::itemFor() (that is why project must be added to m_projects before parsing)

Also the patch changes Info* classes in a way that makes possible to parse files without loading them into KatePart, i.e. without Document object creation. This speeds up project loading. At least on my machine before most of the time was spent in spell checker initialization. We do not need a spell checker for parsing :). To parse a file we use a tiny helper function that loads file contents (getFileContents() in anonymous namespace, parserthread.cpp).

Then it tries to fix a race between parsing and cleaning of Info object by separating the cleaning code into cleanInfo() method and calling it from installParserResults()

With these changes completion works for all bib items and labels in project even if these files are not opened.

Testing

Manual testing

Diffs

  • src/documentinfo.h (16d6e49)
  • src/documentinfo.cpp (923d099)
  • src/kiledocmanager.h (5edc14e)
  • src/kiledocmanager.cpp (aa8a1d8)
  • src/parser/parserthread.cpp (4e80c41)

View Diff