From: Michel L. <mic...@gm...> - 2012-09-07 08:12:33
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18643 ----------------------------------------------------------- As I'm not very familiar with Biblatex, is it true that the only way Biblatex knows whether it should run BibTeX or Biber is through '\usepackage[backend = biber]{biblatex}'? If so, I think it would be better if we parsed that command in order to determine which tool to run. - Michel Ludwig On Sept. 6, 2012, 9:29 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2012, 9:29 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/kilestdtools.cpp 0c6e5f0 > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-07 20:58:56
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18672 ----------------------------------------------------------- Thanks, but actually 'LaTeXOutputHandler' could be used for storing information that results from compilation attempts. 'LaTeXInfo' and 'KileProject' both inherit 'LaTeXOutputHandler', i.e. the LaTeX tool will always have a pointer to the right object (either a simple document or a project depending on what is being compiled). So, you could add methods to LaTeXOutputHandler that would record which bibliography tool was run last. However, I'm not sure that this is the right fix. Kile tries to run BibTeX as the timestamp of the .bbl has changed (probably due to Biblatex rewriting it) and 'updateBibs' returns true. But at the same time, LaTeX didn't print a message saying that biber would have to be rerun. So, wouldn't it be easier to simply rely on these message if Biblatex is used, i.e. in this case nothing printed and we don't run _any_ bibliography tool? - Michel Ludwig On Sept. 7, 2012, 7:38 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2012, 7:38 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/kilestdtools.cpp 0c6e5f0 > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-08 08:10:58
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18679 ----------------------------------------------------------- Thanks! I'd also suggest to give the user the power to override our bib tool detection by adding a new entry to the "Build" menu with a submenu that would allow to choose between "auto-detect", "BibTeX", or "Biblatex" (similar to the "Live Preview" entry). The chosen settings could then be stored in LaTeXOutputHandler as well (which might also need a new name now as it refers to pre-compilation settings as well then :) src/documentinfo.h <http://git.reviewboard.kde.org/r/106363/#comment14762> Could you leave the methods 'public' here? src/outputinfo.h <http://git.reviewboard.kde.org/r/106363/#comment14763> Just to keep the naming convention: writeBibliographyBack... -> setBibliographyBackendToolName readBibliographyBack... -> bibliographyBackendToolName() - Michel Ludwig On Sept. 7, 2012, 10:14 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2012, 10:14 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.cpp 0c6e5f0 > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-08 20:34:48
|
> On Sept. 8, 2012, 8:10 a.m., Michel Ludwig wrote: > > Thanks! > > > > I'd also suggest to give the user the power to override our bib tool detection by adding a new entry to the "Build" menu with a submenu that would allow to choose between "auto-detect", "BibTeX", or "Biblatex" (similar to the "Live Preview" entry). The chosen settings could then be stored in LaTeXOutputHandler as well (which might also need a new name now as it refers to pre-compilation settings as well then :) > > Eugene Shalygin wrote: > This is a good idea! Sould I change class 'KileTool::Manager' to add function like 'bibliographyTool()' or should I add class 'BibliographyManager' (similar to 'LivePreviewManager')? I would add a 'createActions' method to 'KileTool::Manager' which sets up a 'KSelectAction' that allows to choose between the different bibtool modes, and then also add slots like 'updateBibliographyToolMenu', 'bibtexToolSelected', ... to 'KileTool::Manager'. I've added the signal 'currentLaTeXOutputHandlerChanged(LaTeXOutputHandler*)' to 'KileErrorHandler', which you can use to update the menu whenever the current document/project changes. - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18679 ----------------------------------------------------------- On Sept. 7, 2012, 10:14 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2012, 10:14 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.cpp 0c6e5f0 > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-09 23:02:21
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/ ----------------------------------------------------------- (Updated Sept. 9, 2012, 11:02 p.m.) Review request for Kile. Changes ------- Action added to build menu. But I'm not happy with a few things in the patch. 1. String constants with tools names in LaTeXOutputHandler. Can I get these names from somewhere? 2. I do not know how to put KSelectAction into XMLGUI file. Is it possible? 3. Checks in the very beginning of KileTool::Manager::currentLaTeXOutputHandlerChanged. Would it be better to rearrange meny creation and event subscription? Or in any case one can expect a situation when menu does not exist? 4. KileProject::compilationSettingsConfigGroupName. Would be logical to use separate group. But then, LivePreview settings which are very similar, could also be in that group? 5. Just my private opinion (perhaps, casted by the using of IDEs) that user expects these settings in the project options, but not in the build menu. Personally I found LivePreview settings only after looking into the sources :) Description ------- As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. This addresses bug 268047. http://bugs.kde.org/show_bug.cgi?id=268047 Diffs (updated) ----- src/documentinfo.h 27ad09f src/documentinfo.cpp 2c3a3bb src/kile.cpp 8061b54 src/kileproject.h e77035a src/kileproject.cpp dd4087d src/kilestdtools.h 769e8ca src/kilestdtools.cpp 0c6e5f0 src/kiletoolmanager.h 86e2258 src/kiletoolmanager.cpp d06e36f src/outputinfo.h 096c708 src/outputinfo.cpp 8d18adc Diff: http://git.reviewboard.kde.org/r/106363/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Michel L. <mic...@gm...> - 2012-09-10 18:58:39
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- Thanks for changes! Here are a few comments: 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} BibliograhpyBackendSettings bibliographyBackendTool(); void setBibliographyBackendTool(BibliograhpyBackendSettings s); void setAutoDetectedBibliographyBackendTool(const QString& s); QString autoDetectedBibliographyBackendTool() const; Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. src/kile.cpp <http://git.reviewboard.kde.org/r/106363/#comment14921> This should take a KActionCollection* parameter and be moved before "setupGUI". src/kilestdtools.cpp <http://git.reviewboard.kde.org/r/106363/#comment14905> I'd leave the logic for detecting the backend here, i.e. in the LaTeX tool class. src/kiletoolmanager.cpp <http://git.reviewboard.kde.org/r/106363/#comment14920> You can connect each of these actions to appropriate slot, like "biberBackendToolSelected" or "bibliographyBackendAutoDetectionSelection" and then do the appropriate changes in LaTeXOutputHandler. This avoids having to use a special QMap. - Michel Ludwig On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-10 21:30:20
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. Dear Michel, thanks for the review, first of all! 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. Regarding item "2" I'm grateful and will try. 3. Do not understand, sorry. 4. As you wish. 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-11 07:21:19
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-11 11:45:44
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". 5. So for now a in-memory storage in LaTeXInfo is OK? - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-11 12:54:01
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) > > Eugene Shalygin wrote: > 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. > > Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? > > 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". > 5. So for now a in-memory storage in LaTeXInfo is OK? 1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)? The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there. For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT". 3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI. 5. Yes, later I will take care of saving it. - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-11 17:54:15
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) > > Eugene Shalygin wrote: > 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. > > Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? > > 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". > 5. So for now a in-memory storage in LaTeXInfo is OK? > > Michel Ludwig wrote: > 1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)? > The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class > called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there. > For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT". > 3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI. > 5. Yes, later I will take care of saving it. 1. How I'm supposed to update kilerc file to add "class=Bibliography" to all bib tools? Earlier you have said me that current update system will be abandomed for Kile 3.0. So what should I do? Also, I'd like to extend ToolConfigPair to add execuable name. May I? That wil simplify my task and I think can not injure in other places? Or I can make ToolConfigPair a real class and derive BibtoolConfig from it. - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-11 18:28:31
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) > > Eugene Shalygin wrote: > 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. > > Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? > > 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". > 5. So for now a in-memory storage in LaTeXInfo is OK? > > Michel Ludwig wrote: > 1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)? > The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class > called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there. > For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT". > 3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI. > 5. Yes, later I will take care of saving it. > > Eugene Shalygin wrote: > 1. How I'm supposed to update kilerc file to add "class=Bibliography" to all bib tools? Earlier you have said me that current update system will be abandomed for Kile 3.0. So what should I do? > Also, I'd like to extend ToolConfigPair to add execuable name. May I? That wil simplify my task and I think can not injure in other places? Or I can make ToolConfigPair a real class and derive BibtoolConfig from it. When running Kile 3 for the first time, the tools will be reloaded, i.e. the tools from the local config file will be overwritten by the tools defined in "kilestdtools.rc". I meant that this reloading could be done in a more intelligent way by not touching user-created tools, for example. You can reset the tools manually in the configuration dialog under Tools / Build / Restore default tools. ToolConfigPair is a proper class. You could simply declare a new class that derives from it. But is it really necessary? Wouldn't it be sufficient to add a method like QString commandFor(const QString& tool, const QString& cfg, KConfig *config) { return config->group(groupFor(tool, cfg)).readEntry("command", ""); } to the tool manager? (The tools have pointers to the appropriate config and tool manager objects) - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-11 19:38:13
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) > > Eugene Shalygin wrote: > 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. > > Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? > > 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". > 5. So for now a in-memory storage in LaTeXInfo is OK? > > Michel Ludwig wrote: > 1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)? > The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class > called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there. > For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT". > 3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI. > 5. Yes, later I will take care of saving it. > > Eugene Shalygin wrote: > 1. How I'm supposed to update kilerc file to add "class=Bibliography" to all bib tools? Earlier you have said me that current update system will be abandomed for Kile 3.0. So what should I do? > Also, I'd like to extend ToolConfigPair to add execuable name. May I? That wil simplify my task and I think can not injure in other places? Or I can make ToolConfigPair a real class and derive BibtoolConfig from it. > > Michel Ludwig wrote: > When running Kile 3 for the first time, the tools will be reloaded, i.e. the tools from the local config file will be overwritten by the tools defined in "kilestdtools.rc". I meant that this reloading could be done in a more intelligent way by not touching user-created tools, for example. You can reset the tools manually in the configuration dialog under Tools / Build / Restore default tools. > > ToolConfigPair is a proper class. You could simply declare a new class that derives from it. But is it really necessary? Wouldn't it be sufficient to add a method like > > QString commandFor(const QString& tool, const QString& cfg, KConfig *config) > { > return config->group(groupFor(tool, cfg)).readEntry("command", ""); > } > > to the tool manager? (The tools have pointers to the appropriate config and tool manager objects) So I just update .rc files. OK. ToolConfigPair is a QPair, right? Derived class would have attirbutes "first", "second", and "command". That is not beautiful. And I really would like too have the command loaded since I need it every time I detect backend. Isn't that method "commandFor" too complicated for that? From the other hand, I have to react on tools config changes... But tool names can be changed as well as commands, so reloading is needed anyway. - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-11 20:16:40
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) > > Eugene Shalygin wrote: > 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. > > Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? > > 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". > 5. So for now a in-memory storage in LaTeXInfo is OK? > > Michel Ludwig wrote: > 1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)? > The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class > called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there. > For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT". > 3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI. > 5. Yes, later I will take care of saving it. > > Eugene Shalygin wrote: > 1. How I'm supposed to update kilerc file to add "class=Bibliography" to all bib tools? Earlier you have said me that current update system will be abandomed for Kile 3.0. So what should I do? > Also, I'd like to extend ToolConfigPair to add execuable name. May I? That wil simplify my task and I think can not injure in other places? Or I can make ToolConfigPair a real class and derive BibtoolConfig from it. > > Michel Ludwig wrote: > When running Kile 3 for the first time, the tools will be reloaded, i.e. the tools from the local config file will be overwritten by the tools defined in "kilestdtools.rc". I meant that this reloading could be done in a more intelligent way by not touching user-created tools, for example. You can reset the tools manually in the configuration dialog under Tools / Build / Restore default tools. > > ToolConfigPair is a proper class. You could simply declare a new class that derives from it. But is it really necessary? Wouldn't it be sufficient to add a method like > > QString commandFor(const QString& tool, const QString& cfg, KConfig *config) > { > return config->group(groupFor(tool, cfg)).readEntry("command", ""); > } > > to the tool manager? (The tools have pointers to the appropriate config and tool manager objects) > > Eugene Shalygin wrote: > So I just update .rc files. OK. > > ToolConfigPair is a QPair, right? Derived class would have attirbutes "first", "second", and "command". That is not beautiful. And I really would like too have the command loaded since I need it every time I detect backend. Isn't that method "commandFor" too complicated for that? From the other hand, I have to react on tools config changes... But tool names can be changed as well as commands, so reloading is needed anyway. What we can also do is to add a method "ToolConfigPair bibliographyToolFromCommand(const QString& s)" or so to the tool manager. Basically, the tool manager would keep track of the bibliography tool and allow for lookups. Whenever the tools are reloaded, it could update the bibliography tools list as well (but for that you still have to move the "KileTool::Factory::readStandardToolConfig()" method to the tool manager, where it really belongs, and then rebuild the lists) And the tool manager would also need to listen to the "configChanged" signal from the configuration manager (in "configurationmanager.h"). - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-11 20:31:27
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) > > Eugene Shalygin wrote: > 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. > > Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? > > 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". > 5. So for now a in-memory storage in LaTeXInfo is OK? > > Michel Ludwig wrote: > 1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)? > The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class > called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there. > For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT". > 3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI. > 5. Yes, later I will take care of saving it. > > Eugene Shalygin wrote: > 1. How I'm supposed to update kilerc file to add "class=Bibliography" to all bib tools? Earlier you have said me that current update system will be abandomed for Kile 3.0. So what should I do? > Also, I'd like to extend ToolConfigPair to add execuable name. May I? That wil simplify my task and I think can not injure in other places? Or I can make ToolConfigPair a real class and derive BibtoolConfig from it. > > Michel Ludwig wrote: > When running Kile 3 for the first time, the tools will be reloaded, i.e. the tools from the local config file will be overwritten by the tools defined in "kilestdtools.rc". I meant that this reloading could be done in a more intelligent way by not touching user-created tools, for example. You can reset the tools manually in the configuration dialog under Tools / Build / Restore default tools. > > ToolConfigPair is a proper class. You could simply declare a new class that derives from it. But is it really necessary? Wouldn't it be sufficient to add a method like > > QString commandFor(const QString& tool, const QString& cfg, KConfig *config) > { > return config->group(groupFor(tool, cfg)).readEntry("command", ""); > } > > to the tool manager? (The tools have pointers to the appropriate config and tool manager objects) > > Eugene Shalygin wrote: > So I just update .rc files. OK. > > ToolConfigPair is a QPair, right? Derived class would have attirbutes "first", "second", and "command". That is not beautiful. And I really would like too have the command loaded since I need it every time I detect backend. Isn't that method "commandFor" too complicated for that? From the other hand, I have to react on tools config changes... But tool names can be changed as well as commands, so reloading is needed anyway. > > Michel Ludwig wrote: > What we can also do is to add a method "ToolConfigPair bibliographyToolFromCommand(const QString& s)" or so to the tool manager. Basically, the tool manager would keep track of the bibliography tool and allow for lookups. Whenever the tools are reloaded, it could update the bibliography tools list as well (but for that you still have to move the "KileTool::Factory::readStandardToolConfig()" method to the tool manager, where it really belongs, and then rebuild the lists) > > And the tool manager would also need to listen to the "configChanged" signal from the configuration manager (in "configurationmanager.h"). Thanks! Everything is clear except the question with the ToolPairClass. I do not need mappping from tool command to name. I need a list of possible commands binded to tools configurations. If I do not extend TooConfiglPair I'll have a separate list of commands. And a implicit map in bibliographyToolFromCommand(). This is almost the same but less structured. If you are caring about overhead (command is not needed everywhere), we can add parameters to toolsWithConfigurationsBasedOnClass() to will specify which parts of toll config to load. P.S. I see that patch becomes big enough. Perhaps I'd better submit it in several parts (like introducing "Bibliography" class, changing ToolConfigPair, adding methods to tool manager, and finally the bibtool selection)? - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-11 20:53:56
|
> On Sept. 10, 2012, 6:58 p.m., Michel Ludwig wrote: > > Thanks for changes! Here are a few comments: > > > > 1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler: > > enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND} > > > > BibliograhpyBackendSettings bibliographyBackendTool(); > > void setBibliographyBackendTool(BibliograhpyBackendSettings s); > > > > void setAutoDetectedBibliographyBackendTool(const QString& s); > > QString autoDetectedBibliographyBackendTool() const; > > > > Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class. > > 2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag). > > 3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.). > > 4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names. > > 5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog. > > > > It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best. > > Eugene Shalygin wrote: > Dear Michel, > > thanks for the review, first of all! > 1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings. > Regarding item "2" I'm grateful and will try. > 3. Do not understand, sorry. > 4. As you wish. > 5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice > > Michel Ludwig wrote: > 1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)). > As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there. > If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :)) > 3. This is basically just the same as 2. The technology to do that is called KXMLGUI. > 5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :) > > Eugene Shalygin wrote: > 1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human. If we want strings, then LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. > > Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think? > > 3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly". > 5. So for now a in-memory storage in LaTeXInfo is OK? > > Michel Ludwig wrote: > 1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)? > The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class > called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there. > For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT". > 3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI. > 5. Yes, later I will take care of saving it. > > Eugene Shalygin wrote: > 1. How I'm supposed to update kilerc file to add "class=Bibliography" to all bib tools? Earlier you have said me that current update system will be abandomed for Kile 3.0. So what should I do? > Also, I'd like to extend ToolConfigPair to add execuable name. May I? That wil simplify my task and I think can not injure in other places? Or I can make ToolConfigPair a real class and derive BibtoolConfig from it. > > Michel Ludwig wrote: > When running Kile 3 for the first time, the tools will be reloaded, i.e. the tools from the local config file will be overwritten by the tools defined in "kilestdtools.rc". I meant that this reloading could be done in a more intelligent way by not touching user-created tools, for example. You can reset the tools manually in the configuration dialog under Tools / Build / Restore default tools. > > ToolConfigPair is a proper class. You could simply declare a new class that derives from it. But is it really necessary? Wouldn't it be sufficient to add a method like > > QString commandFor(const QString& tool, const QString& cfg, KConfig *config) > { > return config->group(groupFor(tool, cfg)).readEntry("command", ""); > } > > to the tool manager? (The tools have pointers to the appropriate config and tool manager objects) > > Eugene Shalygin wrote: > So I just update .rc files. OK. > > ToolConfigPair is a QPair, right? Derived class would have attirbutes "first", "second", and "command". That is not beautiful. And I really would like too have the command loaded since I need it every time I detect backend. Isn't that method "commandFor" too complicated for that? From the other hand, I have to react on tools config changes... But tool names can be changed as well as commands, so reloading is needed anyway. > > Michel Ludwig wrote: > What we can also do is to add a method "ToolConfigPair bibliographyToolFromCommand(const QString& s)" or so to the tool manager. Basically, the tool manager would keep track of the bibliography tool and allow for lookups. Whenever the tools are reloaded, it could update the bibliography tools list as well (but for that you still have to move the "KileTool::Factory::readStandardToolConfig()" method to the tool manager, where it really belongs, and then rebuild the lists) > > And the tool manager would also need to listen to the "configChanged" signal from the configuration manager (in "configurationmanager.h"). > > Eugene Shalygin wrote: > Thanks! > Everything is clear except the question with the ToolPairClass. I do not need mappping from tool command to name. I need a list of possible commands binded to tools configurations. If I do not extend TooConfiglPair I'll have a separate list of commands. And a implicit map in bibliographyToolFromCommand(). This is almost the same but less structured. If you are caring about overhead (command is not needed everywhere), we can add parameters to toolsWithConfigurationsBasedOnClass() to will specify which parts of toll config to load. > > P.S. I see that patch becomes big enough. Perhaps I'd better submit it in several parts (like introducing "Bibliography" class, changing ToolConfigPair, adding methods to tool manager, and finally the bibtool selection)? > I'd prefer to leave ToolConfigPair as it is. It's such a nice class :) In the tool manager you could store a map from command -> ToolConfigPair which you can rebuild whenever the tools are changed. For that you can use the "toolsWithConfigurationsBasedOnClass" method, look up the command with the method from above and also the default configuration of the tool with "configName(...)". "bibliographyToolFromCommand()" would simply become a hash lookup then. I don't mind big patches. So I leave this up to you :) - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18804 ----------------------------------------------------------- On Sept. 9, 2012, 11:02 p.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2012, 11:02 p.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-13 20:42:46
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/ ----------------------------------------------------------- (Updated Sept. 13, 2012, 8:42 p.m.) Review request for Kile. Changes ------- Avaliable bibliogaphy tools are loaded from configuration. Actions for every tool are created. No more "ifs". There are few questions, still :) 1. Is it possible (by Kile design) that a user changes tools configurations and compilation is running in the same time? For the patch it means: do we need to check (in Manager::bibliographyBackendSelectedByUser()) does selected by user bibtol exist? It will be no harm if we do not check, but that can confuse a user. 2. We have slot Manager::reloadToolsMap() which should be invoked after tools configuration has changed. So far I do not do this. Shall I just invoke it from tools editor widget? Or we can subscribe to some magic event? In other aspects I'm much more happy with these results of our discussions. Hope you will be to :) Description ------- As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. This addresses bug 268047. http://bugs.kde.org/show_bug.cgi?id=268047 Diffs (updated) ----- src/data/kilestdtools-win.rc 85e70d4 src/data/kilestdtools.rc 765a604 src/documentinfo.h 27ad09f src/documentinfo.cpp 2c3a3bb src/kile.cpp 8061b54 src/kileproject.h e77035a src/kileproject.cpp dd4087d src/kilestdtools.h 769e8ca src/kilestdtools.cpp 0c6e5f0 src/kiletool.h 2b6c035 src/kiletool.cpp 4e2fe8d src/kiletoolmanager.h 86e2258 src/kiletoolmanager.cpp d06e36f src/kileui.rc e9f2b92 src/outputinfo.h 096c708 src/outputinfo.cpp 8d18adc src/widgets/toolconfigwidget.cpp 1845291 Diff: http://git.reviewboard.kde.org/r/106363/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Eugene S. <eug...@gm...> - 2012-09-14 10:37:33
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/ ----------------------------------------------------------- (Updated Sept. 14, 2012, 10:37 a.m.) Review request for Kile. Changes ------- 1. Fix acion objects ownership handling 2. Reaload bibliogrpahy tools after user edits 3. White spaces and includes (which left from previous version of a patch) cleanup Description ------- As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. This addresses bug 268047. http://bugs.kde.org/show_bug.cgi?id=268047 Diffs (updated) ----- src/data/kilestdtools-win.rc 85e70d4 src/data/kilestdtools.rc 765a604 src/documentinfo.h 27ad09f src/documentinfo.cpp 2c3a3bb src/kile.cpp 8061b54 src/kileproject.h e77035a src/kileproject.cpp dd4087d src/kilestdtools.h 769e8ca src/kilestdtools.cpp 0c6e5f0 src/kiletool.h 2b6c035 src/kiletool.cpp 4e2fe8d src/kiletoolmanager.h 86e2258 src/kiletoolmanager.cpp d06e36f src/kileui.rc e9f2b92 src/outputinfo.h 096c708 src/outputinfo.cpp 8d18adc src/widgets/toolconfigwidget.cpp 1845291 Diff: http://git.reviewboard.kde.org/r/106363/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Michel L. <mic...@gm...> - 2012-09-14 20:30:50
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18977 ----------------------------------------------------------- Thanks for the changes! It looks very good already, but to improve the long-term maintainability/performance of the code, I've added some comments below. But what do we actually do if two different configurations of a tool have the same command but different parameters otherwise. How do we know which configuration to choose? src/kileproject.h <http://git.reviewboard.kde.org/r/106363/#comment15048> I don't think there is a need to declare a class variable for this string. A preprocessor definition will be enough. src/kilestdtools.cpp <http://git.reviewboard.kde.org/r/106363/#comment15049> There's no need to declare a variable here. src/kilestdtools.cpp <http://git.reviewboard.kde.org/r/106363/#comment15050> You can write "ToolConfigPair(QString("BibTeX"), QString())" instead of "defaultTool" here. src/kiletool.h <http://git.reviewboard.kde.org/r/106363/#comment15051> These methods should no longer be required. src/kiletoolmanager.h <http://git.reviewboard.kde.org/r/106363/#comment15054> Instead of exposing the map, could you use methods to access it instead? like "containsBibliographyTool" or "findTool...". The code will be easier to maintain that way. src/kiletoolmanager.h <http://git.reviewboard.kde.org/r/106363/#comment15052> Can you use a QHash instead of a QMap? This is faster. src/kiletoolmanager.cpp <http://git.reviewboard.kde.org/r/106363/#comment15053> Can you use "NULL" instead of "0"? src/outputinfo.h <http://git.reviewboard.kde.org/r/106363/#comment15045> Can you make it take "const ToolConfigPair& p" as argument? In that way we don't have to convert back and forth between a string and a ToolConfigPair. src/outputinfo.h <http://git.reviewboard.kde.org/r/106363/#comment15046> Can you make it return a ToolConfigPair? src/outputinfo.h <http://git.reviewboard.kde.org/r/106363/#comment15047> Same as above. You can return a ToolConfigPair("","") to indicate that no tool has been set by the user. src/widgets/toolconfigwidget.cpp <http://git.reviewboard.kde.org/r/106363/#comment15044> Can you connect this slot to the "configChanged()" signal from the configuration manager ("configurationmanager.h") instead of calling it directly? - Michel Ludwig On Sept. 14, 2012, 10:37 a.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2012, 10:37 a.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/data/kilestdtools-win.rc 85e70d4 > src/data/kilestdtools.rc 765a604 > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletool.h 2b6c035 > src/kiletool.cpp 4e2fe8d > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/kileui.rc e9f2b92 > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > src/widgets/toolconfigwidget.cpp 1845291 > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-16 20:23:56
|
> On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > Thanks for the changes! It looks very good already, but to improve the long-term maintainability/performance of the code, I've added some comments below. > > > > But what do we actually do if two different configurations of a tool have the same command but different parameters otherwise. How do we know which configuration to choose? Thank you for the time and review! I can understand your point of view. Agreeing with some of your points, I think that thre is a better solution for others. I reply in particular comments. Concerning "same commands - different tools" question I belieave that only user can make a desicion and that is why we are introducing user overrides of the autodetection, aren't we? > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/kileproject.h, line 264 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85363#file85363line264> > > > > I don't think there is a need to declare a class variable for this string. > > > > A preprocessor definition will be enough. Do I understand correctly that you do not want to pollute the class namespace? We can move the constant to an anonymous namespace in .cpp file. > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/kilestdtools.cpp, line 339 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85366#file85366line339> > > > > There's no need to declare a variable here. I disagree. Having static variable we can return it more efficiently comparing to creation of QStrings(). Moreover, declaration can be moved in the function body and be used as a return value some in some additional if-branch if (if we need to change the algorithm in that way). > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/kiletool.h, line 39 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85367#file85367line39> > > > > These methods should no longer be required. No, it is needed for action name construction. > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/kiletoolmanager.h, line 123 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85369#file85369line123> > > > > Instead of exposing the map, could you use methods to access it instead? like "containsBibliographyTool" or "findTool...". > > The code will be easier to maintain that way. I doubt how to do that. Iteration over tools is needed. If we have containsBibliographyTool() and findTool() that means we perform search in the map tools several times instead of iteration. What would help is forEachTool(Predicate, Action). Do you want it? > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/kiletoolmanager.h, line 180 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85369#file85369line180> > > > > Can you use a QHash instead of a QMap? This is faster. Not necessarily: with our 3 and a half bib tools, string comparison can not be slower than calculation of a hash value from a key. I do not think that we will have hudreds of bib tools. > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/kiletoolmanager.cpp, line 103 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85370#file85370line103> > > > > Can you use "NULL" instead of "0"? Maybe, even nullptr? > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/outputinfo.h, line 148 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85372#file85372line148> > > > > Can you make it take "const ToolConfigPair& p" as argument? In that way we don't have to convert back and forth between a string and a ToolConfigPair. To this and the next: yes, it would be nice, but for doing that one has to move ToolConfigPair declaration out of "kiletool.h" which includes "outputinfo.h" to avoid circular includes. Where can I put it? > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/widgets/toolconfigwidget.cpp, line 313 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85374#file85374line313> > > > > Can you connect this slot to the "configChanged()" signal from the configuration manager ("configurationmanager.h") instead of calling it directly? Thank you, done. I did not know about this signal. > On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > src/outputinfo.h, line 149 > > <http://git.reviewboard.kde.org/r/106363/diff/6/?file=85372#file85372line149> > > > > Can you make it return a ToolConfigPair? - Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18977 ----------------------------------------------------------- On Sept. 14, 2012, 10:37 a.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2012, 10:37 a.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/data/kilestdtools-win.rc 85e70d4 > src/data/kilestdtools.rc 765a604 > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletool.h 2b6c035 > src/kiletool.cpp 4e2fe8d > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/kileui.rc e9f2b92 > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > src/widgets/toolconfigwidget.cpp 1845291 > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-17 07:41:36
|
> On Sept. 14, 2012, 8:30 p.m., Michel Ludwig wrote: > > Thanks for the changes! It looks very good already, but to improve the long-term maintainability/performance of the code, I've added some comments below. > > > > But what do we actually do if two different configurations of a tool have the same command but different parameters otherwise. How do we know which configuration to choose? > > Eugene Shalygin wrote: > Thank you for the time and review! > > I can understand your point of view. Agreeing with some of your points, I think that thre is a better solution for others. I reply in particular comments. > > Concerning "same commands - different tools" question I belieave that only user can make a desicion and that is why we are introducing user overrides of the autodetection, aren't we? > Yes, I just thought a simpler logic would be enough. We wouldn't need to iterate through the tools, but we would just have a simple mapping e.g., from "bibtex" to the default bibtool, and from "biber" to the default Biber tool. But I don't mind anymore, we can also leave the iteration. - Michel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review18977 ----------------------------------------------------------- On Sept. 14, 2012, 10:37 a.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2012, 10:37 a.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/data/kilestdtools-win.rc 85e70d4 > src/data/kilestdtools.rc 765a604 > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletool.h 2b6c035 > src/kiletool.cpp 4e2fe8d > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/kileui.rc e9f2b92 > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > src/widgets/toolconfigwidget.cpp 1845291 > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Michel L. <mic...@gm...> - 2012-09-17 07:29:32
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review19042 ----------------------------------------------------------- src/kileproject.h <http://git.reviewboard.kde.org/r/106363/#comment15113> Above all, I don't want to waste memory. If you declare a class variable, it will end up in the executable, unnecessarily occupying a few bytes. All you need it for is to define some string locally in KileProject. Hence, something like #define SETTINGS_GROUP_NAME "bla" will be enough, and no variable declaration is needed. src/kilestdtools.cpp <http://git.reviewboard.kde.org/r/106363/#comment15112> What you probably meant was return a reference to a ToolConfigPair (ToolConfigPair&). Unlike Java, C++ does not consider every object variable to be a reference. Here, if you don't put "static", the performance will be exactly the same as most likely the variable will be optimised away by the compiler. With "static", the variable won't end up on the stack but in a different location in the executable, which probably means that it will have to be loaded whenever it is accessed, and causing a slight slowdown. If you absolutely want a variable there, then I wouldn't declare it as "static". src/kiletool.h <http://git.reviewboard.kde.org/r/106363/#comment15122> Could you use the same code as in "LivePreviewManager::buildLivePreviewMenu"? (which actually isn't right as there should be a call to i18n when the text of the action is constructed) The texts of the tool selection actions should be the same in the live preview submenu and the bibliography submenu. src/kiletoolmanager.h <http://git.reviewboard.kde.org/r/106363/#comment15120> Hehe, what I ideally want are a few methods with self-explanatory names that make it clear what they are supposed to do. With exposing the map, I will probably look at the code again in a few months/years and it will take me some time to figure out what's going on ;) There's no need to worry about performance here as we are only dealing with a handful of tools anyway. The need for maintainability of the code outweighs any performance gains here. src/kiletoolmanager.h <http://git.reviewboard.kde.org/r/106363/#comment15121> The Qt documentation states that "QHash provides faster lookups than QMap". QMap will also compute hash values of strings, but it also provides some ordering guarantee on the keys, which we don't really need. I agree that there probably won't be any performance difference, but it is nevertheless a good habit to use the most appropriate classes everywhere. In this case that would be QHash. src/kiletoolmanager.cpp <http://git.reviewboard.kde.org/r/106363/#comment15114> nullptr is part of C++11, which is not yet supported by all of the compilers that are supposed to be able to compile KDE software. So, we cannot use it yet. - Michel Ludwig On Sept. 14, 2012, 10:37 a.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2012, 10:37 a.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/data/kilestdtools-win.rc 85e70d4 > src/data/kilestdtools.rc 765a604 > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 8061b54 > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletool.h 2b6c035 > src/kiletool.cpp 4e2fe8d > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/kileui.rc e9f2b92 > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > src/widgets/toolconfigwidget.cpp 1845291 > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |
From: Eugene S. <eug...@gm...> - 2012-09-20 16:29:38
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/ ----------------------------------------------------------- (Updated Sept. 20, 2012, 4:29 p.m.) Review request for Kile. Changes ------- 1. Remove QMap of bib tools. 2. Make compactName/fromCompactName compatible with strings in LivePreview menu. Michel, I still believe that QMap is not only faster on small collections ( it does not calculate or store hashes, it uses only "operator <") but also uses less memory, because it does not need to store array of hashes and pointers. Conserning QString constant: as it uses 2 bytes per symbol, and, of course it is more memory than #define. But is it significant? A few bytes... but reading is easier - constant is localized. Description ------- As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. This addresses bug 268047. http://bugs.kde.org/show_bug.cgi?id=268047 Diffs (updated) ----- src/data/kilestdtools-win.rc 85e70d4 src/data/kilestdtools.rc 765a604 src/documentinfo.h 27ad09f src/documentinfo.cpp 2c3a3bb src/kile.cpp 8061b54 src/kileproject.h e77035a src/kileproject.cpp dd4087d src/kilestdtools.h 769e8ca src/kilestdtools.cpp 0c6e5f0 src/kiletool.h 2b6c035 src/kiletool.cpp 4e2fe8d src/kiletoolmanager.h 86e2258 src/kiletoolmanager.cpp d06e36f src/kileui.rc e9f2b92 src/outputinfo.h 096c708 src/outputinfo.cpp 8d18adc src/widgets/toolconfigwidget.cpp 1845291 Diff: http://git.reviewboard.kde.org/r/106363/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Eugene S. <eug...@gm...> - 2012-09-22 11:33:00
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/ ----------------------------------------------------------- (Updated Sept. 22, 2012, 11:32 a.m.) Review request for Kile. Changes ------- Update to reflect change in master branch: Add missing i18n call in 'LivePreviewManager::buildLivePreviewMenu'. Add the same for bib tools menu. Description ------- As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. This addresses bug 268047. http://bugs.kde.org/show_bug.cgi?id=268047 Diffs (updated) ----- src/data/kilestdtools-win.rc 647b2e1 src/data/kilestdtools.rc c3bf120 src/documentinfo.h 27ad09f src/documentinfo.cpp 2c3a3bb src/kile.cpp 79320ca src/kileproject.h e77035a src/kileproject.cpp dd4087d src/kilestdtools.h 769e8ca src/kilestdtools.cpp 0c6e5f0 src/kiletool.h 2b6c035 src/kiletool.cpp 4e2fe8d src/kiletoolmanager.h 86e2258 src/kiletoolmanager.cpp d06e36f src/kileui.rc e9f2b92 src/livepreview.cpp 2ac3236 src/outputinfo.h 096c708 src/outputinfo.cpp 8d18adc src/widgets/toolconfigwidget.cpp 1845291 Diff: http://git.reviewboard.kde.org/r/106363/diff/ Testing ------- Manual testing Thanks, Eugene Shalygin |
From: Michel L. <mic...@gm...> - 2012-09-22 16:03:42
|
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/#review19308 ----------------------------------------------------------- Ship it! Ship It! - Michel Ludwig On Sept. 22, 2012, 11:32 a.m., Eugene Shalygin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106363/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2012, 11:32 a.m.) > > > Review request for Kile. > > > Description > ------- > > As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography > > This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}. > Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also. > I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside. > > > This addresses bug 268047. > http://bugs.kde.org/show_bug.cgi?id=268047 > > > Diffs > ----- > > src/data/kilestdtools-win.rc 647b2e1 > src/data/kilestdtools.rc c3bf120 > src/documentinfo.h 27ad09f > src/documentinfo.cpp 2c3a3bb > src/kile.cpp 79320ca > src/kileproject.h e77035a > src/kileproject.cpp dd4087d > src/kilestdtools.h 769e8ca > src/kilestdtools.cpp 0c6e5f0 > src/kiletool.h 2b6c035 > src/kiletool.cpp 4e2fe8d > src/kiletoolmanager.h 86e2258 > src/kiletoolmanager.cpp d06e36f > src/kileui.rc e9f2b92 > src/livepreview.cpp 2ac3236 > src/outputinfo.h 096c708 > src/outputinfo.cpp 8d18adc > src/widgets/toolconfigwidget.cpp 1845291 > > Diff: http://git.reviewboard.kde.org/r/106363/diff/ > > > Testing > ------- > > Manual testing > > > Thanks, > > Eugene Shalygin > > |