D20958: New menu of syntax highlighting in the status bar

cullmann added a comment. Coding style wise, could one change: 1. this->XXX() => XXX() 2. void functions should not have "return;" at the end, we don't do that REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg,

D20958: New menu of syntax highlighting in the status bar

nibags marked 2 inline comments as done. nibags added a comment. I changed QListWidget by QListView in D21712 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg, ngraham, cullmann Cc:

D20958: New menu of syntax highlighting in the status bar

dhaumann added a comment. Btw, I think this is a really nice improvement! Keep it coming :-) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg, ngraham, cullmann Cc: dhaumann, anthonyfieroni, cullmann, ngraham, loh.tar,

D20958: New menu of syntax highlighting in the status bar

nibags added a comment. Now it's OK REPOSITORY R39 KTextEditor BRANCH new-mode-menu REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg, ngraham, cullmann Cc: dhaumann, anthonyfieroni, cullmann, ngraham, loh.tar, kwrite-devel,

D20958: New menu of syntax highlighting in the status bar

nibags closed this revision. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg, ngraham, cullmann Cc: dhaumann, anthonyfieroni, cullmann, ngraham, loh.tar, kwrite-devel, kde-frameworks-devel, domson, michaelh, bruns,

D20958: New menu of syntax highlighting in the status bar

nibags updated this revision to Diff 57905. nibags added a comment. - Add delimiters « » REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20958?vs=57841=57905 BRANCH new-mode-menu REVISION DETAIL https://phabricator.kde.org/D20958 AFFECTED FILES

D20958: New menu of syntax highlighting in the status bar

nibags updated this revision to Diff 57841. nibags added a comment. - Fixes the menu alignment of the previous update REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20958?vs=57819=57841 BRANCH new-mode-menu REVISION DETAIL

D20958: New menu of syntax highlighting in the status bar

nibags updated this revision to Diff 57819. nibags added a comment. - Fixes: Some fixes for locations: - Before, the word wrap was only applied in spaces and, in languages such as German, there are large words that pass under the scroll bar. This is corrected. - Improves the

D20958: New menu of syntax highlighting in the status bar

loh.tar added a comment. >> I could add a "Recent" section to the beginning of the menu, it would be useful. > > Nice! Just to be clear. That should to be work over session borders, not only the current one. Should you do it, here some bug report

D20958: New menu of syntax highlighting in the status bar

cullmann added a comment. Ok, thanks! REPOSITORY R39 KTextEditor BRANCH new-mode-menu REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg, ngraham, cullmann Cc: dhaumann, anthonyfieroni, cullmann, ngraham, loh.tar, kwrite-devel,

D20958: New menu of syntax highlighting in the status bar

nibags added a comment. I'm going to move it to QListView (or QTreeView) in a future diff, I have checked and it isn't complicated to do. Now I want to test it a bit more before doing the commit. REPOSITORY R39 KTextEditor BRANCH new-mode-menu REVISION DETAIL

D20958: New menu of syntax highlighting in the status bar

cullmann accepted this revision. cullmann added a comment. I am ok with having this as is. If one can move it to QListView, nice, but that could be done in an extra request. Thanks for this great improvement. REPOSITORY R39 KTextEditor BRANCH new-mode-menu REVISION DETAIL

D20958: New menu of syntax highlighting in the status bar

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. Thanks, this is now perfect from my UI perspective. I'll let the KTextEditor folks do the code review part. :) REPOSITORY R39 KTextEditor BRANCH new-mode-menu REVISION DETAIL

D20958: New menu of syntax highlighting in the status bar

nibags updated this revision to Diff 57618. nibags added a comment. - Allow to select an item with a single click, regardless of the configuration of the system. I had forgotten that it's possible to change the settings for the click/double-click actions, now I correct it. Then it

D20958: New menu of syntax highlighting in the status bar

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. Still needs the menu items to apply and close on single-click even when using the systemwide double-click mode. The option itself in System Settings Provides a clue regarding

D20958: New menu of syntax highlighting in the status bar

nibags marked 8 inline comments as done. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg Cc: dhaumann, anthonyfieroni, cullmann, ngraham, loh.tar, kwrite-devel, kde-frameworks-devel, domson, michaelh, bruns, demsking, sars

D20958: New menu of syntax highlighting in the status bar

nibags added a comment. > Can you make this a QListView? I once heard the QListWidget will be deprecated in Qt6, rule of thumb is: always use Q*View instead of Q*Widget, since this then will also work in QML. I used QListWidget because it's more simpler to implement, but in that case

D20958: New menu of syntax highlighting in the status bar

nibags updated this revision to Diff 57561. nibags added a comment. - Update: - Select the search text when showing the menu. - Also use Shift, Alt or Meta + Return to select an item without hiding the menu. - Fix style in signals/slots. - Use constant variable for icon size. -

D20958: New menu of syntax highlighting in the status bar

dhaumann added a comment. My comments are very vague, but I think this patch could even be better. Only did a review half through due to lack of time. INLINE COMMENTS > katemodemenulist.h:141 > + */ > +class ModeListWidget : public QListWidget > +{ Can you make this a

D20958: New menu of syntax highlighting in the status bar

anthonyfieroni added a comment. dymanic_cast is expensive, prefer qobject_cast if you want to check result against nullptr, if you don't want - static_cast. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg Cc:

D20958: New menu of syntax highlighting in the status bar

anthonyfieroni added inline comments. INLINE COMMENTS > katemodemenulist.cpp:65 > +m_list->setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff); > +m_list->setIconSize(QSize(16, 16)); > +m_list->setResizeMode(QListView::Adjust); As well for icon size. > katemodemenulist.cpp:129 > +

D20958: New menu of syntax highlighting in the status bar

loh.tar added a comment. > I could add a "Recent" section to the beginning of the menu, it would be useful. Nice! Just to be clear. That should to be work over session borders, not only the current one. >> The blue frame (in dark theme) looks compared to the other plain menus

D20958: New menu of syntax highlighting in the status bar

ngraham added a comment. In D20958#460130 , @nibags wrote: > > Single-clicking on an item in the list should select it and close the menu; double-click doesn't gain us anything here > > To me the double-click works correctly. It may also be

D20958: New menu of syntax highlighting in the status bar

nibags added a comment. Thanks for the comments, in a while I will update the diff with the suggestions/corrections. > The only thing I would like to have is an auto generated "most often used" list. That was also suggested somewhere. This way you could quickly change between preferred

D20958: New menu of syntax highlighting in the status bar

cullmann added a comment. As first feedback: much better than current state :) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg Cc: cullmann, ngraham, loh.tar, kwrite-devel, kde-frameworks-devel, domson, michaelh,

D20958: New menu of syntax highlighting in the status bar

ngraham added a comment. In D20958#459676 , @loh.tar wrote: > - The blue frame (in dark theme) looks compared to the other plain menus slightly strange, any possibility to remove this? This's a generic issue with the selection border

D20958: New menu of syntax highlighting in the status bar

loh.tar added a comment. > When the menu is opened, clear any text in the search field left over from a prior search For my taste would that the usability only worsen, this way may not be usual but works charming. Perhaps may a change to select the field in that case a

D20958: New menu of syntax highlighting in the status bar

ngraham added a comment. OMG so much better! Adding a search field is a godsend. I have a few UI comments: - When the menu is opened, clear any text in the search field left over from a prior search - Single-clicking on an item in the list should select it and close the menu;

D20958: New menu of syntax highlighting in the status bar

ngraham added a reviewer: VDG. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate, #vdg Cc: loh.tar, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D20958: New menu of syntax highlighting in the status bar

loh.tar added a comment. Wow, look very nice :-) The only thing I would like to have is an auto generated "most often used" list. That was also suggested somewhere. This way you could quickly change between preferred styles instead to search for them. Your code looks lovely and well

D20958: New menu of syntax highlighting in the status bar

nibags edited the summary of this revision. nibags added reviewers: KTextEditor, Kate. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D20958 To: nibags, #ktexteditor, #kate Cc: kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns, demsking, cullmann,

D20958: New menu of syntax highlighting in the status bar

nibags created this revision. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. nibags requested review of this revision. REPOSITORY R39 KTextEditor BRANCH new-mode-menu REVISION DETAIL https://phabricator.kde.org/D20958 AFFECTED FILES