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
