From: Eugene S. <eug...@gm...> - 2012-09-25 15:06:20
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/ ----------------------------------------------------------- Review request for Kile. 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. 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) With these changes completion works for all bib items and labels in project even if these files are not opened. However, project loading time increases significantly. Diffs ----- src/kiledocmanager.cpp 1d652c1 Diff: http://git.reviewboard.kde.org/r/106567/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Eugene S. <eug...@gm...> - 2012-09-26 20:49:58
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/ ----------------------------------------------------------- (Updated Sept. 26, 2012, 8:49 p.m.) Review request for Kile. Changes ------- Change 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 these changes most of the time was spent in spell checker initialization. We do not need a spell checker for parsing :) Seems like race condition does not occur anymore after splitting code that cleans up Info object into separate method and calling it in installParserOutput(). The last one is supposed to run from the main thread, right? As an overwiev, now Kile parses all projet items on loading, doing this faster, and completion works without opening respective documents 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. 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) With these changes completion works for all bib items and labels in project even if these files are not opened. However, project loading time increases significantly. Diffs (updated) ----- src/parser/latexparser.cpp ea0ae0b src/parser/parserthread.h c41123b src/parser/parserthread.cpp ad38819 src/kiledocmanager.cpp 1d652c1 src/documentinfo.cpp 2c3a3bb src/documentinfo.h 27ad09f Diff: http://git.reviewboard.kde.org/r/106567/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Eugene S. <eug...@gm...> - 2012-09-26 21:04:33
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/ ----------------------------------------------------------- (Updated Sept. 26, 2012, 9:04 p.m.) Review request for Kile. Changes ------- Identation fix-up. Sorry for the noise 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. 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) With these changes completion works for all bib items and labels in project even if these files are not opened. However, project loading time increases significantly. Diffs (updated) ----- src/parser/latexparser.cpp ea0ae0b src/parser/parserthread.h c41123b src/parser/parserthread.cpp ad38819 src/kiledocmanager.cpp 1d652c1 src/documentinfo.cpp 2c3a3bb src/documentinfo.h 27ad09f Diff: http://git.reviewboard.kde.org/r/106567/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Eugene S. <eug...@gm...> - 2012-09-26 21:12:18
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/ ----------------------------------------------------------- (Updated Sept. 26, 2012, 9:12 p.m.) Review request for Kile. Changes ------- update description Description (updated) ------- 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. Diffs ----- src/parser/latexparser.cpp ea0ae0b src/parser/parserthread.h c41123b src/parser/parserthread.cpp ad38819 src/kiledocmanager.cpp 1d652c1 src/documentinfo.cpp 2c3a3bb src/documentinfo.h 27ad09f Diff: http://git.reviewboard.kde.org/r/106567/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Eugene S. <eug...@gm...> - 2012-09-27 10:08:39
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19489 ----------------------------------------------------------- Returning to the race conditions question, I would like to note that having somethig like waitParsingFinished() might help. This can be called before rebuilding project tree. However, while waiting for empty parsing queue is obvious and simple, that does not help much because events that drive Info* objects updates are asynchronous. If one would have a centralized place for that, then it would be possible to check _its_ queue. Perhaps yet anothe thread that only installs parsing results (By the way what does mean the comment to parsingComplete() event: ".. ownership transfered to ... slots(s)". Slots? Which one will be an owner? A separate installThread would sort it out). Without such mechanism, call to buildProjectTree() in projectOpen() is nearly useless. - Eugene Shalygin On Sept. 26, 2012, 9:12 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2012, 9:12 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/parser/latexparser.cpp ea0ae0b > src/parser/parserthread.h c41123b > src/parser/parserthread.cpp ad38819 > src/kiledocmanager.cpp 1d652c1 > src/documentinfo.cpp 2c3a3bb > src/documentinfo.h 27ad09f > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-05 17:33:33
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/ ----------------------------------------------------------- (Updated Oct. 5, 2012, 5:33 p.m.) Review request for Kile. Changes ------- documentContents property is added to TextInfo. Seems to be working. Do not understand the note "... but please check that 'update' will indeed pass the contents to the parser.". How it could be possible to not pass contents to the parser? Is there one more hidden thread? :) Please note Info::clearStruct(), that you had not not included before. This is to eliminate race, which, of course, does not happen when we are loading Kate documents (the loading is slower than parsing). 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. Diffs (updated) ----- src/documentinfo.h 16d6e49 src/documentinfo.cpp 923d099 src/kiledocmanager.h 5edc14e src/kiledocmanager.cpp aa8a1d8 src/parser/parserthread.cpp 4e80c41 Diff: http://git.reviewboard.kde.org/r/106567/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Michel L. <mic...@gm...> - 2012-10-05 20:27:26
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- 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? src/documentinfo.cpp <http://git.reviewboard.kde.org/r/106567/#comment15838> 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. src/kiledocmanager.cpp <http://git.reviewboard.kde.org/r/106567/#comment15837> It looks like this is still needed somewhere... src/kiledocmanager.cpp <http://git.reviewboard.kde.org/r/106567/#comment15836> Could you use 'KIO::NetAccess::download' here? And maybe do everything in one method? It should make the code easier to understand. - Michel Ludwig On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-05 22:17:38
|
> On Oct. 5, 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? > > 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 Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. 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 Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 1561 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line1561> > > > > It looks like this is still needed somewhere... 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? > On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 2570 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line2570> > > > > Could you use 'KIO::NetAccess::download' here? And maybe do everything in one method? > > > > It should make the code easier to understand. 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... - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-10-06 08:37:10
|
> On Oct. 5, 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? > > > > 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. 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 Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. > > 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? Ok, let's do it this way. > On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 2570 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line2570> > > > > Could you use 'KIO::NetAccess::download' here? And maybe do everything in one method? > > > > It should make the code easier to understand. > > 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... 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 ;) - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-06 13:31:31
|
> On Oct. 5, 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? > > > > 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. > > 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. 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 Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. > > 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? > > Michel Ludwig wrote: > Ok, let's do it this way. 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 Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 2570 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line2570> > > > > Could you use 'KIO::NetAccess::download' here? And maybe do everything in one method? > > > > It should make the code easier to understand. > > 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... > > 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 ;) 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. - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-10-06 15:46:26
|
> On Oct. 5, 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? > > > > 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. > > 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. > > 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. Ok, but that's a different issue. Your fix was right for the reason that you had emptied 'Info::cleanStruct()'. > On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. > > 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? > > Michel Ludwig wrote: > Ok, let's do it this way. > > 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. 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? > On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 2570 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line2570> > > > > Could you use 'KIO::NetAccess::download' here? And maybe do everything in one method? > > > > It should make the code easier to understand. > > 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... > > 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 ;) > > 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. 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... - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-06 15:56:55
|
> On Oct. 5, 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? > > > > 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. > > 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. > > 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. > > 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 Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. > > 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? > > Michel Ludwig wrote: > Ok, let's do it this way. > > 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. > > 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 Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 2570 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line2570> > > > > Could you use 'KIO::NetAccess::download' here? And maybe do everything in one method? > > > > It should make the code easier to understand. > > 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... > > 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 ;) > > 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. > > 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 :)) > On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 1561 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line1561> > > > > It looks like this is still needed somewhere... > > 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? - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-10-06 17:06:59
|
> On Oct. 5, 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? > > > > 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. > > 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. > > 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. > > Michel Ludwig wrote: > Ok, but that's a different issue. Your fix was right for the reason that you had emptied 'Info::cleanStruct()'. > > Eugene Shalygin wrote: > 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()? No, that's exactly the point. It is unnecessary to clear any information before the parsing takes place. > On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. > > 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? > > Michel Ludwig wrote: > Ok, let's do it this way. > > 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. > > 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? > > Eugene Shalygin wrote: > 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. Calling 'KileDocument::Manager::urlFor' in 'DocumentParserThread::addDocument' should now work for project items as well. > On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/kiledocmanager.cpp, line 1561 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88512#file88512line1561> > > > > It looks like this is still needed somewhere... > > 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? > > Eugene Shalygin wrote: > Any comment about this? That seems fine. - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-06 18:36:18
|
> On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. > > 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? > > Michel Ludwig wrote: > Ok, let's do it this way. > > 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. > > 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? > > Eugene Shalygin wrote: > 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. > > Michel Ludwig wrote: > Calling 'KileDocument::Manager::urlFor' in 'DocumentParserThread::addDocument' should now work for project items as well. Thank you! But seems like this will take more time to rework the patch then I expected. After merging your commits, I have a crash with the following backtrace: #5 0x000000000047f5fc in QList<QString>::QList (this=0x7fff1fc31f20, l=...) at /usr/include/qt4/QtCore/qlist.h:122 #6 0x0000000000476dcf in QStringList::QStringList (this=0x7fff1fc31f20, l=...) at /usr/include/qt4/QtCore/qstringlist.h:71 #7 0x0000000000529131 in KileDocument::Info::labels (this=0x0) at /home/eugene/develop/KDE/kile/src/documentinfo.h:127 #8 0x0000000000527698 in KileInfo::retrieveList (this=0x2970a78, getit=(QStringList (KileDocument::Info::*)(const KileDocument::Info * const)) 0x52910a <KileDocument::Info::labels() const>, docinfo=0x0) at /home/eugene/develop/KDE/kile/src/kileinfo.cpp:244 #9 0x0000000000527977 in KileInfo::allLabels (this=0x2970a78, info=0x0) at /home/eugene/develop/KDE/kile/src/kileinfo.cpp:263 #10 0x00000000005861e5 in KileWidget::StructureView::showReferences (this=0x47cb780, ki=0x2970a78) at /home/eugene/develop/KDE/kile/src/widgets/structurewidget.cpp:568 #11 0x000000000058a356 in KileWidget::StructureWidget::updateAfterParsing (this=0x3aaf9e0, info=0x47c93d0, items=...) at /home/eugene/develop/KDE/kile/src/widgets/structurewidget.cpp:1009 #12 0x00000000005ccbeb in KileDocument::Manager::handleParsingComplete (this=0x29bd820, url=..., output=0x47de0a0) at /home/eugene/develop/KDE/kile/src/kiledocmanager.cpp:2069 #13 0x00000000005d1c22 in KileDocument::Manager::qt_static_metacall (_o=0x29bd820, _c=QMetaObject::InvokeMetaMethod, _id=98, _a=0x7fe913061d20) at /home/eugene/develop/KDE/kile/build/debug/src/kiledocmanager.moc:354 #14 0x00007fe924990c2e in QObject::event(QEvent*) () from /usr/lib64/qt4/libQtCore.so.4 #15 0x00007fe9238c88dc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib64/qt4/libQtGui.so.4 #16 0x00007fe9238ca65a in QApplication::notify(QObject*, QEvent*) () from /usr/lib64/qt4/libQtGui.so.4 #17 0x00007fe925650c16 in KApplication::notify(QObject*, QEvent*) () from /usr/lib64/libkdeui.so.5 #18 0x00007fe92497bfae in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/lib64/qt4/libQtCore.so.4 #19 0x00007fe92497f7d1 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib64/qt4/libQtCore.so.4 #20 0x00007fe9249a94a3 in postEventSourceDispatch(_GSource*, int (*)(void*), void*) () from /usr/lib64/qt4/libQtCore.so.4 #21 0x00007fe91e387035 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0 #22 0x00007fe91e387368 in g_main_context_iterate.isra.23 () from /usr/lib64/libglib-2.0.so.0 #23 0x00007fe91e387424 in g_main_context_iteration () from /usr/lib64/libglib-2.0.so.0 #24 0x00007fe9249a9636 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtCore.so.4 #25 0x00007fe9239637de in QGuiEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtGui.so.4 #26 0x00007fe92497fc7f in QCoreApplication::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtCore.so.4 #27 0x00000000005c91f8 in KileDocument::Manager::projectOpen (this=0x29bd820, url=..., step=0, max=2, openProjectItemViews=false) at /home/eugene/develop/KDE/kile/src/kiledocmanager.cpp:1652 #28 0x00000000004e5b88 in Kile::restoreFilesAndProjects (this=0x2970a30, allowRestore=true) at /home/eugene/develop/KDE/kile/src/kile.cpp:1240 #29 0x00000000004d4534 in Kile::Kile (this=0x2970a30, allowRestore=true, parent=0x0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /home/eugene/develop/KDE/kile/src/kile.cpp:362 #30 0x00000000004fd985 in main (argc=1, argv=0x7fff1fc33f78) at /home/eugene/develop/KDE/kile/src/main.cpp:153 Perhaps, just to set Info for all items before calling projectOpenItem()? - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-10-06 18:54:25
|
> On Oct. 5, 2012, 8:27 p.m., Michel Ludwig wrote: > > src/documentinfo.cpp, line 551 > > <http://git.reviewboard.kde.org/r/106567/diff/4/?file=88510#file88510line551> > > > > 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. > > 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? > > Michel Ludwig wrote: > Ok, let's do it this way. > > 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. > > 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? > > Eugene Shalygin wrote: > 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. > > Michel Ludwig wrote: > Calling 'KileDocument::Manager::urlFor' in 'DocumentParserThread::addDocument' should now work for project items as well. > > Eugene Shalygin wrote: > Thank you! But seems like this will take more time to rework the patch then I expected. > After merging your commits, I have a crash with the following backtrace: > > #5 0x000000000047f5fc in QList<QString>::QList (this=0x7fff1fc31f20, l=...) at /usr/include/qt4/QtCore/qlist.h:122 > #6 0x0000000000476dcf in QStringList::QStringList (this=0x7fff1fc31f20, l=...) at /usr/include/qt4/QtCore/qstringlist.h:71 > #7 0x0000000000529131 in KileDocument::Info::labels (this=0x0) at /home/eugene/develop/KDE/kile/src/documentinfo.h:127 > #8 0x0000000000527698 in KileInfo::retrieveList (this=0x2970a78, getit=(QStringList (KileDocument::Info::*)(const KileDocument::Info * const)) 0x52910a <KileDocument::Info::labels() const>, docinfo=0x0) at /home/eugene/develop/KDE/kile/src/kileinfo.cpp:244 > #9 0x0000000000527977 in KileInfo::allLabels (this=0x2970a78, info=0x0) at /home/eugene/develop/KDE/kile/src/kileinfo.cpp:263 > #10 0x00000000005861e5 in KileWidget::StructureView::showReferences (this=0x47cb780, ki=0x2970a78) at /home/eugene/develop/KDE/kile/src/widgets/structurewidget.cpp:568 > #11 0x000000000058a356 in KileWidget::StructureWidget::updateAfterParsing (this=0x3aaf9e0, info=0x47c93d0, items=...) at /home/eugene/develop/KDE/kile/src/widgets/structurewidget.cpp:1009 > #12 0x00000000005ccbeb in KileDocument::Manager::handleParsingComplete (this=0x29bd820, url=..., output=0x47de0a0) at /home/eugene/develop/KDE/kile/src/kiledocmanager.cpp:2069 > #13 0x00000000005d1c22 in KileDocument::Manager::qt_static_metacall (_o=0x29bd820, _c=QMetaObject::InvokeMetaMethod, _id=98, _a=0x7fe913061d20) at /home/eugene/develop/KDE/kile/build/debug/src/kiledocmanager.moc:354 > #14 0x00007fe924990c2e in QObject::event(QEvent*) () from /usr/lib64/qt4/libQtCore.so.4 > #15 0x00007fe9238c88dc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib64/qt4/libQtGui.so.4 > #16 0x00007fe9238ca65a in QApplication::notify(QObject*, QEvent*) () from /usr/lib64/qt4/libQtGui.so.4 > #17 0x00007fe925650c16 in KApplication::notify(QObject*, QEvent*) () from /usr/lib64/libkdeui.so.5 > #18 0x00007fe92497bfae in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/lib64/qt4/libQtCore.so.4 > #19 0x00007fe92497f7d1 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib64/qt4/libQtCore.so.4 > #20 0x00007fe9249a94a3 in postEventSourceDispatch(_GSource*, int (*)(void*), void*) () from /usr/lib64/qt4/libQtCore.so.4 > #21 0x00007fe91e387035 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0 > #22 0x00007fe91e387368 in g_main_context_iterate.isra.23 () from /usr/lib64/libglib-2.0.so.0 > #23 0x00007fe91e387424 in g_main_context_iteration () from /usr/lib64/libglib-2.0.so.0 > #24 0x00007fe9249a9636 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtCore.so.4 > #25 0x00007fe9239637de in QGuiEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtGui.so.4 > #26 0x00007fe92497fc7f in QCoreApplication::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtCore.so.4 > #27 0x00000000005c91f8 in KileDocument::Manager::projectOpen (this=0x29bd820, url=..., step=0, max=2, openProjectItemViews=false) at /home/eugene/develop/KDE/kile/src/kiledocmanager.cpp:1652 > #28 0x00000000004e5b88 in Kile::restoreFilesAndProjects (this=0x2970a30, allowRestore=true) at /home/eugene/develop/KDE/kile/src/kile.cpp:1240 > #29 0x00000000004d4534 in Kile::Kile (this=0x2970a30, allowRestore=true, parent=0x0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /home/eugene/develop/KDE/kile/src/kile.cpp:362 > #30 0x00000000004fd985 in main (argc=1, argv=0x7fff1fc33f78) at /home/eugene/develop/KDE/kile/src/main.cpp:153 > > Perhaps, just to set Info for all items before calling projectOpenItem()? Yes, that seems necessary. It looks like one of the event loops that is started from within 'projectOpen' handled the completion of the parsing of a project file but during the reconstruction of the structure view it encountered a project item without a TextInfo. Can you also add a non-null check at line 244 of "kileinfo.cpp" (inside 'KileInfo::retrieveList')? - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review19983 ----------------------------------------------------------- On Oct. 5, 2012, 5:33 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2012, 5:33 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 923d099 > src/kiledocmanager.h 5edc14e > src/kiledocmanager.cpp aa8a1d8 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-06 19:20:00
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/ ----------------------------------------------------------- (Updated Oct. 6, 2012, 7:19 p.m.) Review request for Kile. Changes ------- Avoid using Info::url() Regarding Manager::loadFileContents(): I did the best I can wothout KTemporaryFile and exceptions. Result does not look good :) Will be thankfull if you show me how to improve it. My brain just chants "Exceptions!" :) 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. Diffs (updated) ----- src/documentinfo.h 16d6e49 src/documentinfo.cpp 8ec5bff src/kiledocmanager.h 6013186 src/kiledocmanager.cpp 2cad021 src/parser/parserthread.cpp 4e80c41 Diff: http://git.reviewboard.kde.org/r/106567/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Michel L. <mic...@gm...> - 2012-10-06 21:27:15
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review20026 ----------------------------------------------------------- Ship it! Do you want to work on error handling as well now? I think it would be good if errors that occur during the loading are not immediately reported to the user but only all the projects have been opened. Then, there could be some dialog popping up listing the files that couldn't have been opened. What do you think? (and exception are not really used in KDE/Qt code ;)) - Michel Ludwig On Oct. 6, 2012, 7:19 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2012, 7:19 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 8ec5bff > src/kiledocmanager.h 6013186 > src/kiledocmanager.cpp 2cad021 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-06 22:06:43
|
> On Oct. 6, 2012, 9:27 p.m., Michel Ludwig wrote: > > Do you want to work on error handling as well now? I think it would be good if errors that occur during the loading are not immediately reported to the user but only all the projects have been opened. Then, there could be some dialog popping up listing the files that couldn't have been opened. What do you think? > > > > (and exception are not really used in KDE/Qt code ;)) Happy to see that this changes finally landed! Thank you so much for all the help and changes to the code! Good to see that even without RAII/exceptions one can use tricks like the one for local files in NetAccess::download() and turn Manager::loadTextURLContents() into a readable function. Just as a little help for a reader it would be better to move the comment to the line with actual action and to mention the trick in download() (as their authors have used so non self-describing name), please? However, I do not see any harm from exception in an application like Kile: it is hard to find a compiler that does not support them, we a not a library and thus do not have users those must use exceptions also, and we know exactly what are the speed/memory requirements for a particular function. Or nowadays KDE developers invented some additional reasons to not use exceptions? ;) I not, Kile will only benefits from a more clean code. --- Yes, loading errors handling must be added. But I think that one should start from making possible either to use remote project files or to use files outside of the project directory in a project tree. I would be exceptionally happy with the second one, cause I have many shared images, tables and other fragments. I'm even not speaking about bibliography databases... So what are the problems with that? Why this is disallowed in Kile? Knowing the problems I can think about possible solutions. After that the error handling code can be tested :) - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review20026 ----------------------------------------------------------- On Oct. 6, 2012, 7:19 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2012, 7:19 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 8ec5bff > src/kiledocmanager.h 6013186 > src/kiledocmanager.cpp 2cad021 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-10-07 09:27:23
|
> On Oct. 6, 2012, 9:27 p.m., Michel Ludwig wrote: > > Do you want to work on error handling as well now? I think it would be good if errors that occur during the loading are not immediately reported to the user but only all the projects have been opened. Then, there could be some dialog popping up listing the files that couldn't have been opened. What do you think? > > > > (and exception are not really used in KDE/Qt code ;)) > > Eugene Shalygin wrote: > Happy to see that this changes finally landed! Thank you so much for all the help and changes to the code! > > Good to see that even without RAII/exceptions one can use tricks like the one for local files in NetAccess::download() and turn Manager::loadTextURLContents() into a readable function. Just as a little help for a reader it would be better to move the comment to the line with actual action and to mention the trick in download() (as their authors have used so non self-describing name), please? > > However, I do not see any harm from exception in an application like Kile: it is hard to find a compiler that does not support them, we a not a library and thus do not have users those must use exceptions also, and we know exactly what are the speed/memory requirements for a particular function. Or nowadays KDE developers invented some additional reasons to not use exceptions? ;) I not, Kile will only benefits from a more clean code. > > --- > > Yes, loading errors handling must be added. But I think that one should start from making possible either to use remote project files or to use files outside of the project directory in a project tree. I would be exceptionally happy with the second one, cause I have many shared images, tables and other fragments. I'm even not speaking about bibliography databases... > > So what are the problems with that? Why this is disallowed in Kile? Knowing the problems I can think about possible solutions. > > After that the error handling code can be tested :) Sorry, but we cannot really use exceptions in Kile if Qt wasn't designed for that. The biggest obstacle is probably to make exceptions compatible with signals/slots. At least one has to be very, very careful when doing that. Another disadvantage is that executables that use exceptions are apparently up to 20% larger. I wasn't involved with Kile when the project handling code was written. My guess is that they wanted to have everything under one directory to make the invocation of LaTeX easier. How do you do it? Do you modify TEXINPUTS? What about live preview? Does it work on such projects? It feels like this could be a major change. It might be better to leave this for Kile 3.1 as I want to finish version 3.0 asap. - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review20026 ----------------------------------------------------------- On Oct. 6, 2012, 7:19 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2012, 7:19 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 8ec5bff > src/kiledocmanager.h 6013186 > src/kiledocmanager.cpp 2cad021 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-10-08 13:48:38
|
> On Oct. 6, 2012, 9:27 p.m., Michel Ludwig wrote: > > Do you want to work on error handling as well now? I think it would be good if errors that occur during the loading are not immediately reported to the user but only all the projects have been opened. Then, there could be some dialog popping up listing the files that couldn't have been opened. What do you think? > > > > (and exception are not really used in KDE/Qt code ;)) > > Eugene Shalygin wrote: > Happy to see that this changes finally landed! Thank you so much for all the help and changes to the code! > > Good to see that even without RAII/exceptions one can use tricks like the one for local files in NetAccess::download() and turn Manager::loadTextURLContents() into a readable function. Just as a little help for a reader it would be better to move the comment to the line with actual action and to mention the trick in download() (as their authors have used so non self-describing name), please? > > However, I do not see any harm from exception in an application like Kile: it is hard to find a compiler that does not support them, we a not a library and thus do not have users those must use exceptions also, and we know exactly what are the speed/memory requirements for a particular function. Or nowadays KDE developers invented some additional reasons to not use exceptions? ;) I not, Kile will only benefits from a more clean code. > > --- > > Yes, loading errors handling must be added. But I think that one should start from making possible either to use remote project files or to use files outside of the project directory in a project tree. I would be exceptionally happy with the second one, cause I have many shared images, tables and other fragments. I'm even not speaking about bibliography databases... > > So what are the problems with that? Why this is disallowed in Kile? Knowing the problems I can think about possible solutions. > > After that the error handling code can be tested :) > > Michel Ludwig wrote: > Sorry, but we cannot really use exceptions in Kile if Qt wasn't designed for that. The biggest obstacle is probably to make exceptions compatible with signals/slots. At least one has to be very, very careful when doing that. Another disadvantage is that executables that use exceptions are apparently up to 20% larger. > > I wasn't involved with Kile when the project handling code was written. My guess is that they wanted to have everything under one directory to make the invocation of LaTeX easier. How do you do it? Do you modify TEXINPUTS? What about live preview? Does it work on such projects? > > It feels like this could be a major change. It might be better to leave this for Kile 3.1 as I want to finish version 3.0 asap. It is hard to have resource managment compatible with any events model as far as you can not be sure that you have only one subscriber per event. But then why should one use events? And in any RPC due to its asynchronous nature exception or any other error on the remote side is something which is hard to handle. So, I mean that we love exceptions not because of that kind of problems :) And size ob exceutables is just the size of your natural coe plus errors handling code, either this code is exceptions handling or return values handling or whatever. In case of GCC exceptions additionally produce so called shadow stack which is not that large. Thus the more cleaner exceptional situations handling code wins. But you are responcible for the Kile, so you decide. I'm not insisting. I use \input{} with some files which are outside of the project directory. And \addbibresource{} also can accept any file. Actually, \addbibresource is the most often place where I want to include somethig from outside of the project. So far, as I wrote, I create symlinks to bibliography DBs in the project directory. Perhaps, it is possible just to adjust parsing that it will add these outer files to dependencies? Or, maybe, it does it already... Do I understand correctly, that only preview and archive are consumers of this project policy? TEXINPUTS? Isn't it a gun of a bit large calibre for the task? - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106567/#review20026 ----------------------------------------------------------- On Oct. 6, 2012, 7:19 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106567/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2012, 7:19 p.m.) > > > Review request for Kile. > > > 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. > > > Diffs > ----- > > src/documentinfo.h 16d6e49 > src/documentinfo.cpp 8ec5bff > src/kiledocmanager.h 6013186 > src/kiledocmanager.cpp 2cad021 > src/parser/parserthread.cpp 4e80c41 > > Diff: http://git.reviewboard.kde.org/r/106567/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |