D16054: Show partial path in Tabswitcher Ctrl+Tab list to distinguish equally named files

[Dominik Haumann]

dhaumann added a comment.


  Hi Gregor, I just did a post-review again, and noticed several issues I think we should address. I have a patch ready for most of the stuff, but did not yet post it on Phabricator, since I indeed use KTextEditor::Document as member pointer in the FileListItem, and its constructor now reads the icon etc from the document, which destroys the tsttestapp demo. So the comments are just here to make you aware of the issues. We can have the real discussion when I post the patch.

INLINE COMMENTS

> tabswitcher.cpp:144
>      // add to model
> -    auto item = new QStandardItem(iconForDocument(document), document->documentName());
> +    auto item = new detail::FilenameListItem(
> +                                            iconForDocument(document),

Do you see the memory leak? item is never deleted, since you create a copy in ::insertRow().

> tabswitcher.h:142
>      KTextEditor::MainWindow *m_mainWindow;
> -    QStandardItemModel * m_model;
> +    detail::TabswitcherFilesModel * m_model;
>      QSet<KTextEditor::Document *> m_documents;

Since we also have a TabSwitcherTreeView which is not in the detail namespace, I think putting the model into the detail namespace adds more inconsistency than it solves. In fact, Given the tabswitcher plugin is just 3 files, it is arguable what is a "detail" and what not. In other words: Overengineering? :-)

> tabswitcherfilesmodel.cpp:33
> +     */
> +    QString longestCommonPrefix(std::vector<QString>& strs) {
> +        int n = INT_MAX;

What license is this? Since we seem to reuse logs of code, this is important :-)

> tabswitcherfilesmodel.h:33
> + */
> +struct FilenameListItem : public QStandardItem
> +{

Indeed using a QStandardItem when NOT using a QStandardItemModel is more confusing than it helps, since some data is fetched directly from the item's member variables, and some data via the data() accessor. I strongly suggest to not do this: Since we derive from QAbstractTableModel now, I'd argue that using QStandardItem is wrong in terms of "that's not how it was designed".

Mixing ->data() and direct member access is rather confusing.

REPOSITORY
  R40 Kate

REVISION DETAIL
  https://phabricator.kde.org/D16054

To: gregormi, #kate, #vdg, cullmann
Cc: cullmann, abetts, thsurrel, dhaumann, ngraham, kwrite-devel, michaelh, demsking, sars
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20181017/732ac57f/attachment-0001.html>