https://bugs.kde.org/show_bug.cgi?id=442903
Igor Kushnir <igor...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Latest Commit| |https://invent.kde.org/kdev
| |elop/kdevelop/-/commit/1b60
| |b1dd18fb8f26c451c4ede77011e
| |13aa7fcac
Status|ASSIGNED |RESOLVED
Version Fixed In| |5.13.231200
--- Comment #3 from Igor Kushnir <igor...@gmail.com> ---
Git commit 1b60b1dd18fb8f26c451c4ede77011e13aa7fcac by Igor Kushnir.
Committed on 15/09/2023 at 11:57.
Pushed by igorkushnir into branch 'master'.
Overhaul SourceFormatterSelectionEdit
Don't select the first style automatically. The first style in the list
is not likely to match the user's preferred style. Let the user select a
style for each language [s]he cares about. Leave the other languages
with no selected style. Delete config entries of MIME types that belong
to languages with no selected style when settings are saved.
There were several issues with the old behavior replaced in this commit:
1. If the user didn't explicitly select a formatter or a style for a
language, the first formatter in the list and its first style were
preselected and stored in config for that language when settings were
saved. Worse still, the GNU_indent_GNU style was stored for Python
and Python 3 languages, even though this style does not support the
Python languages and cannot be selected in the UI (Bug 442903).
2. It was possible to unselect a style in the UI via Ctrl+Click on the
selected style. But SourceFormatterSelectionEdit completely ignored
such deselection. The last deselected style was considered selected
because QListWidget::currentItem() returned it. Now there is a
well-defined no-style-selected state because:
* QItemSelectionModel::selectedIndexes() is used instead of
QListWidget::currentItem();
* &QListWidget::itemSelectionChanged signal is connected to instead
of &QListWidget::currentRowChanged.
3. The Apply button became enabled when language selection changed, even
though merely selecting a different language does not modify
formatting configuration. This behavior could reinforce possible user
misunderstanding of the configuration: that selecting a language
assigns it to an open project (Bug 358798).
4. The order of plugins in the Formatter combobox was not fixed, because
LanguageSettings::formatters was an unordered QSet. So each time the
Configure KDevelop... dialog was opened, the first (and preselected)
item was unpredictably Artistic Style or Custom Script Formatter.
Invisible bugs fixed in this commit:
1. QMap::iterator::value() was called on the iterator already erased
from QMap SourceFormatterSelectionEditPrivate::formatters in
SourceFormatterSelectionEdit::removeSourceFormatter(). This was
undefined behavior, which hasn't revealed itself yet.
2. SourceFormatterSelectionEdit::deleteStyle() erased the current style
from StyleMap SourceFormatter::styles, but failed to destroy the
SourceFormatterStyle object. This was a memory leak.
Error-prone manual memory management was eliminated:
1. SourceFormatter struct contained a QMap with mapped_type = owning raw
SourceFormatterStyle pointer, which had to be deleted manually. Now
FormatterData class contains a std::map with mapped_type =
SourceFormatterStyle value.
2. Similarly, SourceFormatterSelectionEditPrivate::formatters was a
QMap<QString, SourceFormatter*> with owning raw pointers. Now it is a
std::vector<std::unique_ptr<FormatterData>>.
Optimizations:
1. LanguageSettings::mimetypes contained many duplicates because every
MIME type of each supporting style was unconditionally appended to
this list. SourceFormatterSelectionEdit::loadSettings() read and
SourceFormatterSelectionEdit::saveSettings() wrote a config entry for
each MIME type in this list. Now duplicate MIME types are not added
to LanguageSettings::m_mimeTypes.
2. Most styles return the same built-in list from
SourceFormatterStyle::mimeTypes(). Cache already encountered MIME
lists and don't process duplicates. This optimization requires newly
implemented operator== for SourceFormatterStyle::MimeHighlightPair.
3. Widgets' signals weren't blocked consistently. So the same code often
run repeatedly and redundantly. For example, when
SourceFormatterSelectionEdit::selectStyle() called
QListWidget::setCurrentRow(), and the row actually changed,
selectStyle() was called again. Infinite recursion was prevented by
an early return when the current index did not change inside
QItemSelectionModel::setCurrentIndex().
4. updatePreview(), enableStyleButtons() and changed() were often called
repeatedly and redundantly when SourceFormatterSelectionEdit's public
member functions invoked other public member functions.
5. Store addresses rather than names of formatters and styles as user
data in QComboBox and QListWidget. The pointer stability of these
FormatterData and SourceFormatterStyle objects is already necessary
and guaranteed to prevent invalidating selected formatter and style
pointers. Storing pointers replaces name lookup (search) with direct
object access.
6. Use std::vector instead of QMap, QSet and QList to store small (< 10)
numbers of elements. The impact of this container choice on code
complexity is ambiguous (it has both pros and cons), but it is surely
more efficient.
Other improvements:
1. Hide ui.descriptionLabel if the selected style's description is empty
(which is the case for most styles). This leaves more vertical space
to the style preview widget.
2. Don't disable Language and Formatter comboboxes and the Styles list
widget when they are empty. The user cannot do anything with these
empty widgets anyway. The code is shorter and less error-prone
without the disabling and enabling.
3. Cache the currently selected language in
SourceFormatterSelectionEditPrivate::currentLanguagePtr. The current
language rarely changes, so the cache requires little supporting
code. This data member cache allows to call currentLanguage()
whenever needed instead of caching in a local variable as before:
LanguageSettings& l = d->languages[d->ui.cbLanguages->currentText()];
4. Extract reading {MIME type => selected formatter and style} config
entries into a new class SourceFormatter::ConfigForMimeType and use
it in SourceFormatterController and SourceFormatterSelectionEdit
instead of duplicating the parsing code. ConfigForMimeType's extra
validation made the parsing stricter than it was in
SourceFormatterSelectionEdit, which should be fine: better to ignore
broken config entries than try to use them somehow.
5. If the style stored in the config for a language does not support
this language, don't select it and print a warning.
6. qCDebug => qCWarning when a config entry is broken. Make these
messages more informative.
7. Add assertions to help detect bugs sooner.
8. Add code documentation and comments.
FIXED-IN: 5.13.231200
M +8 -0 kdevplatform/interfaces/isourceformatter.h
M +1 -0 kdevplatform/shell/CMakeLists.txt
A +106 -0 kdevplatform/shell/sourceformatterconfig.h [License:
LGPL(v2.0+)]
M +37 -32 kdevplatform/shell/sourceformattercontroller.cpp
M +9 -22 kdevplatform/shell/sourceformattercontroller.h
M +775 -369 kdevplatform/shell/sourceformatterselectionedit.cpp
M +2 -7 kdevplatform/shell/sourceformatterselectionedit.h
M +1 -1 kdevplatform/shell/sourceformatterselectionedit.ui
https://invent.kde.org/kdevelop/kdevelop/-/commit/1b60b1dd18fb8f26c451c4ede77011e13aa7fcac
--
You are receiving this mail because:
You are watching all bug changes.
