Modernizing Kile code (WIP)
AbandonedPublic
Actions

Authored by ognarb on Sep 25 2018, 8:33 PM.

Details

Reviewers

pino
mludwig

Group Reviewers

Kile

Summary

The purpose of this diff is to modernizing the code from kile and understand the architecture of kile. I'm still looking for small fix and looking for any introduced bug with this modification.

Change contains:

+ The use of the range based loop. I did it semi-automaticly with the help from clang-tidy.
+ Use of default keyword for empty constructor and destructor
+ Simplify some if condition. Ex: if (a) { return true; } else { return false; }
+ Use List::empty or QList::isEmpty instead of comparing the length with 0.
+ Rewrite the plain text to latex converter using QString::replace. Note: I will propably replace it later with a regex.

Notable modification are in:
+ src/plaintolatexconverter.cpp
+ src/kile.cpp:2536

Some strange/buggy code:
+ src/editorextention.cpp:267 allDeleted is always false

Diff Detail

Repository

R468 Kile

Lint

Lint Skipped

Unit

Unit Tests Skipped

ognarb created this revision.Sep 25 2018, 8:33 PM

Restricted Application added a subscriber: Kile. · View Herald TranscriptSep 25 2018, 8:33 PM

ognarb requested review of this revision.Sep 25 2018, 8:33 PM

ognarb added a reviewer: Kile.

Hi, thanks for all the interest in KDE software, and kile in particular.

While I'm not a kile developer, I really think doing a single giant commit with lots of unrelated changes is not a good idea: it is really hard to review, and in case you need to debug a regression introduced with this commit then you need to inspect lots of changes.
So my advice is to split this commit into all the various topics, so it can be reviewed easily.

This revision now requires changes to proceed.Sep 25 2018, 9:38 PM

Thank you for your feedback, I will try to rebase this 'giant' commit :)

Also, please leave the variable names as they are and don't introduce 'auto' everywhere. It's useful to know which type one is working with, even if it means that one has to write longer code.

What are the advantages of the following?

+ Use List::empty or QList::isEmpty instead of comparing the length with 0.

Please also make sure that the code can still be compiled on Windows (MSVC 2015, 2017) and on macOS.

One more question, what is the advantage of removing (empty) destructors and replacing them with 'virtual ~Manager() = default;'?

If one wants to write a real destructor later, one will have to introduce the destructor bodies again anyway.

In D15754#331980, @mludwig wrote:

Also, please leave the variable names as they are and don't introduce 'auto' everywhere. It's useful to know which type one is working with, even if it means that one has to write longer code.

Ok I will change that.

What are the advantages of the following?

+ Use List::empty or QList::isEmpty instead of comparing the length with 0.

A bit faster, because length need to be implemented with a for loop O(n) and empty only need to check if the first element exist O(1).

Please also make sure that the code can still be compiled on Windows (MSVC 2015, 2017) and on macOS.

I only use c++11 feature, so it shouldn't be a problem, but I will try to confirm with VM.

In D15754#331985, @mludwig wrote:

One more question, what is the advantage of removing (empty) destructors and replacing them with 'virtual ~Manager() = default;'?

If one wants to write a real destructor later, one will have to introduce the destructor bodies again anyway.

You are right, default only make sense, with copy and move constructor. I will revert this change.

In D15754#332144, @ognarb wrote:

In D15754#331980, @mludwig wrote:

Also, please leave the variable names as they are and don't introduce 'auto' everywhere. It's useful to know which type one is working with, even if it means that one has to write longer code.

Ok I will change that.

To make myself a little clearer, when you replace an iterator variable (typically called 'i') with a range-loop, can you try to find a better variable name than 'i'? I think it would improve the readability of the code.

What are the advantages of the following?

+ Use List::empty or QList::isEmpty instead of comparing the length with 0.

A bit faster, because length need to be implemented with a for loop O(n) and empty only need to check if the first element exist O(1).

That's true for a naive implementation of a list data structure. However, QList::size also runs in O(1).

See https://code.woboq.org/qt5/qtbase/src/corelib/tools/qlist.h.html#_ZNK9QListData4sizeEv

(and also QLinkedList::length actually)

I think we have to find the right balance between patch complexity, reviewing time, and benefits that result from the patch ;)

I will split this revision in multiple revision.

Large Diff

This large diff affects 132 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents
Changeset List

		Path
A	M	.arcconfig (3 lines)
M		src/abbreviationmanager.h (2 lines)
M		src/abbreviationmanager.cpp (7 lines)
M		src/codecompletion.h (6 lines)
M		src/codecompletion.cpp (85 lines)
M		src/configtester.h (12 lines)
M		src/configtester.cpp (26 lines)
M		src/configurationmanager.h (2 lines)
M		src/configurationmanager.cpp (4 lines)
M		src/convert.h (4 lines)
M		src/convert.cpp (8 lines)
M		src/data/symbols-ng/convertOldSymbolFormat.cpp (5 lines)
M		src/dialogs/findfilesdialog.cpp (6 lines)
M		src/dialogs/latexcommanddialog.cpp (12 lines)
M		src/dialogs/listselector.cpp (13 lines)
M		src/dialogs/managetemplatesdialog.cpp (3 lines)
M		src/dialogs/mathenvironmentdialog.cpp (5 lines)
M		src/dialogs/pdf-wizard/pdfdialog.cpp (10 lines)
M		src/dialogs/projectdialogs.h (2 lines)
M		src/dialogs/projectdialogs.cpp (13 lines)
M		src/dialogs/scriptshortcutdialog.cpp (2 lines)
M		src/dialogs/tabular/newtabulardialog.cpp (25 lines)
M		src/dialogs/texdocumentationdialog.cpp (23 lines)
M		src/dialogs/usermenu/usermenudialog.cpp (2 lines)
M		src/dialogs/usermenu/usermenutree.cpp (4 lines)
M		src/docpart.h (2 lines)
M		src/docpart.cpp (4 lines)
M		src/documentinfo.h (6 lines)
M		src/documentinfo.cpp (64 lines)
M		src/editorcommands.h (2 lines)
M		src/editorcommands.cpp (4 lines)
M		src/editorextension.h (2 lines)
M		src/editorextension.cpp (30 lines)
M		src/editorkeysequencemanager.h (10 lines)
M		src/editorkeysequencemanager.cpp (46 lines)
M		src/errorhandler.cpp (8 lines)
M		src/kile.cpp (119 lines)
M		src/kileactions.h (6 lines)
M		src/kileactions.cpp (27 lines)
M		src/kiledocmanager.cpp (177 lines)
M		src/kileextensions.cpp (5 lines)
M		src/kilehelp.cpp (5 lines)
M		src/kileinfo.cpp (15 lines)
M		src/kilelauncher.h (2 lines)
M		src/kilelauncher.cpp (4 lines)
M		src/kilelyxserver.cpp (28 lines)
M		src/kileproject.cpp (95 lines)
M		src/kilestdactions.cpp (21 lines)
M		src/kilestdtools.h (4 lines)
M		src/kilestdtools.cpp (16 lines)
M		src/kiletool.h (8 lines)
M		src/kiletool.cpp (33 lines)
M		src/kiletoolmanager.h (2 lines)
M		src/kiletoolmanager.cpp (30 lines)
M		src/kileviewmanager.h (4 lines)
M		src/kileviewmanager.cpp (27 lines)
M		src/latexcmd.cpp (12 lines)
M		src/livepreview.cpp (19 lines)
M		src/main.cpp (2 lines)
M		src/outputinfo.h (2 lines)
M		src/outputinfo.cpp (5 lines)
M		src/parser/latexoutputparser.cpp (4 lines)
M		src/plaintolatexconverter.h (4 lines)
M		src/plaintolatexconverter.cpp (20 lines)
M		src/scriptmanager.h (2 lines)
M		src/scriptmanager.cpp (51 lines)
M		src/templates.h (8 lines)
M		src/templates.cpp (30 lines)
M		src/tool_utils.cpp (14 lines)
M		src/userhelp.cpp (9 lines)
M		src/usermenu/usermenu.cpp (15 lines)
M		src/utilities.h (2 lines)
M		src/utilities.cpp (8 lines)
M		src/widgets/abbreviationview.h (2 lines)
M		src/widgets/abbreviationview.cpp (7 lines)
M		src/widgets/categorycombobox.h (2 lines)
M		src/widgets/categorycombobox.cpp (4 lines)
M		src/widgets/codecompletionconfigwidget.h (2 lines)
M		src/widgets/codecompletionconfigwidget.cpp (24 lines)
M		src/widgets/commandview.h (4 lines)
M		src/widgets/commandview.cpp (15 lines)
M		src/widgets/filebrowserwidget.h (2 lines)
M		src/widgets/filebrowserwidget.cpp (8 lines)
M		src/widgets/graphicsconfigwidget.h (2 lines)
M		src/widgets/graphicsconfigwidget.cpp (4 lines)
M		src/widgets/helpconfigwidget.h (2 lines)
M		src/widgets/helpconfigwidget.cpp (4 lines)
M		src/widgets/konsolewidget.h (2 lines)
M		src/widgets/konsolewidget.cpp (4 lines)
M		src/widgets/latexconfigwidget.h (2 lines)
M		src/widgets/latexconfigwidget.cpp (4 lines)
M		src/widgets/latextoolconfigwidget.h (2 lines)
M		src/widgets/latextoolconfigwidget.cpp (4 lines)
M		src/widgets/livepreviewconfigwidget.h (2 lines)
M		src/widgets/livepreviewconfigwidget.cpp (4 lines)
M		src/widgets/logwidget.h (2 lines)
M		src/widgets/logwidget.cpp (29 lines)
M		src/widgets/maintoolconfigwidget.h (2 lines)
M		src/widgets/maintoolconfigwidget.cpp (4 lines)
M		src/widgets/newdocumentwidget.h (2 lines)
M		src/widgets/newdocumentwidget.cpp (4 lines)
M		src/widgets/outputview.h (2 lines)
M		src/widgets/outputview.cpp (4 lines)
M		src/widgets/previewconfigwidget.h (6 lines)
M		src/widgets/previewconfigwidget.cpp (4 lines)
M		src/widgets/previewwidget.h (2 lines)
M		src/widgets/previewwidget.cpp (5 lines)
M		src/widgets/processtoolconfigwidget.h (2 lines)
M		src/widgets/processtoolconfigwidget.cpp (4 lines)
M		src/widgets/progressdialog.h (2 lines)
M		src/widgets/progressdialog.cpp (4 lines)
M		src/widgets/quicktoolconfigwidget.h (2 lines)
M		src/widgets/quicktoolconfigwidget.cpp (4 lines)
M		src/widgets/scriptingconfigwidget.h (2 lines)
M		src/widgets/scriptingconfigwidget.cpp (4 lines)
M		src/widgets/scriptsmanagementwidget.h (2 lines)
M		src/widgets/scriptsmanagementwidget.cpp (17 lines)
M		src/widgets/scrollwidget.h (2 lines)
M		src/widgets/scrollwidget.cpp (4 lines)
M		src/widgets/sidebar.h (2 lines)
M		src/widgets/sidebar.cpp (4 lines)
M		src/widgets/statisticswidget.h (2 lines)
M		src/widgets/statisticswidget.cpp (4 lines)
M		src/widgets/statusbar.h (2 lines)
M		src/widgets/statusbar.cpp (4 lines)
M		src/widgets/structureviewconfigwidget.h (2 lines)
M		src/widgets/structureviewconfigwidget.cpp (4 lines)
M		src/widgets/structurewidget.h (4 lines)
M		src/widgets/structurewidget.cpp (22 lines)
M		src/widgets/symbolview.cpp (4 lines)
M		src/widgets/symbolviewconfigwidget.h (2 lines)
M		src/widgets/symbolviewconfigwidget.cpp (4 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	42325			Sep 25 2018, 8:30 PM	★	★

Modernizing Kile code (WIP)AbandonedPublicActions

Details

Diff Detail

Large Diff

Revision ContentsChangeset List

Diff 42325

.arcconfig

src/abbreviationmanager.h

src/abbreviationmanager.cpp

src/codecompletion.h

src/codecompletion.cpp

src/configtester.h

src/configtester.cpp

src/configurationmanager.h

src/configurationmanager.cpp

src/convert.h

src/convert.cpp

src/data/symbols-ng/convertOldSymbolFormat.cpp

src/dialogs/findfilesdialog.cpp

src/dialogs/latexcommanddialog.cpp

src/dialogs/listselector.cpp

src/dialogs/managetemplatesdialog.cpp

src/dialogs/mathenvironmentdialog.cpp

src/dialogs/pdf-wizard/pdfdialog.cpp

src/dialogs/projectdialogs.h

src/dialogs/projectdialogs.cpp

src/dialogs/scriptshortcutdialog.cpp

src/dialogs/tabular/newtabulardialog.cpp

src/dialogs/texdocumentationdialog.cpp

src/dialogs/usermenu/usermenudialog.cpp

src/dialogs/usermenu/usermenutree.cpp

src/docpart.h

src/docpart.cpp

src/documentinfo.h

src/documentinfo.cpp

src/editorcommands.h

src/editorcommands.cpp

src/editorextension.h

src/editorextension.cpp

src/editorkeysequencemanager.h

src/editorkeysequencemanager.cpp

src/errorhandler.cpp

src/kile.cpp

src/kileactions.h

src/kileactions.cpp

src/kiledocmanager.cpp

src/kileextensions.cpp

src/kilehelp.cpp

src/kileinfo.cpp

src/kilelauncher.h

src/kilelauncher.cpp

src/kilelyxserver.cpp

src/kileproject.cpp

src/kilestdactions.cpp

src/kilestdtools.h

src/kilestdtools.cpp

src/kiletool.h

src/kiletool.cpp

src/kiletoolmanager.h

src/kiletoolmanager.cpp

src/kileviewmanager.h

src/kileviewmanager.cpp

src/latexcmd.cpp

src/livepreview.cpp

src/main.cpp

src/outputinfo.h

src/outputinfo.cpp

src/parser/latexoutputparser.cpp

src/plaintolatexconverter.h

src/plaintolatexconverter.cpp

src/scriptmanager.h

src/scriptmanager.cpp

src/templates.h

src/templates.cpp

src/tool_utils.cpp

src/userhelp.cpp

src/usermenu/usermenu.cpp

src/utilities.h

src/utilities.cpp

src/widgets/abbreviationview.h

Modernizing Kile code (WIP)
AbandonedPublic
Actions

Revision Contents
Changeset List