D11382: Add an option to show tabs from last time when Dolphin starts

[Alexander Saoutkin]

feverfew requested changes to this revision.
feverfew added a comment.
This revision now requires changes to proceed.


  Apologies for the delay in reviewing this. I believe FIXED-IN will unfortunately have to change to 20.08. Also looking at the screenshot, the text and the buttons/input box are misaligned, is this resolvable in this patch, or is that a wider issue with settings? Also note in the commit message "thatfunctionality" -> "that functionality".

INLINE COMMENTS

> global.h:57
> +    /**
> +     * Returns a QVector with full data about all currently-running GUI-capable
> +     * Dolphin instances

`full data` is ambiguous here. What this function does is just return all GUI capable instances, and leaves the `QStringList` empty, (which is space to hold all the URL's an instance currently has open, but isn't filled by this function).

> main.cpp:143
>      QList<QUrl> urls = Dolphin::validateUris(args);
> +    bool urlCommandLineArguments = !urls.isEmpty();
>  

I don't understand the point of this variable, just use `urls.isEmpty()` when needed

> main.cpp:179-185
> +    if (!app.isSessionRestored()) {
> +        KConfigGui::setSessionConfig(QStringLiteral("dolphin"), QStringLiteral("dolphin"));
> +    }
> +
> +    if (!urlCommandLineArguments && (app.isSessionRestored() || GeneralSettings::rememberOpenedTabs()) ) {
> +        const QString serviceName = QStringLiteral("org.kde.dolphin-%1").arg(QCoreApplication::applicationPid());
> +        if (!Dolphin::dolphinInstanceData(serviceName).isEmpty()) {

I believe some comments are required here, I'm struggling to follow what's going on here

> main.cpp:182
> -        } else {
> -           qCWarning(DolphinDebug) << "Unknown class " << className << " in session saved data!";
>          }

Why is the debug output removed? (is it genuinely useless?)

> main.cpp:185
> +        const QString serviceName = QStringLiteral("org.kde.dolphin-%1").arg(QCoreApplication::applicationPid());
> +        if (!Dolphin::dolphinInstanceData(serviceName).isEmpty()) {
> +            const QString className = KXmlGuiWindow::classNameOfToplevel(1);

Why the double negative? I believe removing `!` and `isEmpty()` will have the same effect here.

> startupsettingspage.cpp:66
>      // create 'Home URL' editor
> -    QHBoxLayout* homeUrlBoxLayout = new QHBoxLayout();
> +    m_homeUrlBoxLayoutContainer = new QWidget;
> +    QHBoxLayout* homeUrlBoxLayout = new QHBoxLayout(m_homeUrlBoxLayoutContainer);

Does this compile? Shouldn't `QWidget` actually be `QWidget()`?

> startupsettingspage.cpp:84
>  
> -    QHBoxLayout* buttonBoxLayout = new QHBoxLayout();
> +    m_buttonBoxLayoutContainer = new QWidget;
> +    QHBoxLayout* buttonBoxLayout = new QHBoxLayout(m_buttonBoxLayoutContainer);

Same question here

> startupsettingspage.cpp:159
>  
> +    // Remove saved state state if "remember open tabs" has been turned off
> +    if (!m_rememberOpenedTabsRadioButton->isChecked()) {

`state state`?

REPOSITORY
  R318 Dolphin

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

To: ngraham, #dolphin, #vdg, feverfew, meven, elvisangelaccio, ndavis
Cc: davidedmundson, ndavis, intika, feverfew, kfm-devel, ngraham, broulik, #dolphin, azyx, nikolaik, pberestov, iasensio, fprice, fbampaloukas, alexde, Codezela, meven, spoorun, navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20200408/7abef01f/attachment.htm>