D25484: Enable closing a tab by middle click

[Nathaniel Graham]

ngraham added a comment.


  Thanks, this looks great. It works just fine and the UI seems sane to me. I have some code comments:

INLINE COMMENTS

> shell.cpp:660
> +        m_undoCloseTab->setEnabled( true );
> +        m_closedTabUrls.push_back( url );
>  

prefer `append()`

> shell.cpp:778
> +{
> +    if ( m_closedTabUrls.size() == 0 )
> +        return;

coding style: always use braces

> shell.cpp:778
> +{
> +    if ( m_closedTabUrls.size() == 0 )
> +        return;

`m_closedTabUrls.isEmpty()`

> shell.cpp:783
> +
> +    if ( m_closedTabUrls.size() == 0 )
> +        m_undoCloseTab->setEnabled(false);

ditto

> shell.h:175
>    QList<TabState> m_tabs;
> +  QLinkedList<QUrl> m_closedTabUrls;
>    QAction* m_nextTabAction;

A regular old QList of QUrls would seem to do just fine as it doesn't seem that you're using any functionality present in QLinkedList not in QList.

> shell.rc:8
>     <DefineGroup append="print_merge" name="file_print" />
> +   <Action name="undo-close-tab" group="file_open" />
>    </Menu>

Need to bump the version number at the top of this file anything you change anything

REPOSITORY
  R223 Okular

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

To: bdbai, #vdg
Cc: aacid, davidhurka, ngraham, romangg, ndavis, pino, okular-devel, johnzh, andisa, siddharthmanthan, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20191126/aabeba53/attachment.html>