When loading a session always load the "open recent" list
ClosedPublic

Authored by ahmadsamir on Jun 17 2019, 4:23 PM.

Details

Summary

KateMainWindow: move loading/saving "open recent" files list to separate
functions to enable loading that list when loading a session regardless
of the "save window configuration" setting.

Note that there was no strict need to move the code that saved the list
outside of KateMainWindow::saveProperties(), but this way the code is less
confusing.

BUG: 408499
FIXED-IN: 19.08.0

Test Plan
  • Open kate with a new/anonymous session, and disable "restore window configuration"
  • Open some files to populate the "open recent" menu, then close kate
  • Open kate again with a new/anon. session; check if the "open recent" menu has been populated with the previously opened files
  • Load a different session in the same window; check if the "open recent" menu has been populated with the files list from that session
  • Repeat the same steps but with "restore window configuration" enabled

Diff Detail

Repository
R40 Kate
Branch
session-window-config (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13700
Build 13718: arc lint + arc unit
ahmadsamir created this revision.Jun 17 2019, 4:23 PM
Restricted Application added a project: Kate. · View Herald TranscriptJun 17 2019, 4:23 PM
ahmadsamir requested review of this revision.Jun 17 2019, 4:23 PM
cfeck added a subscriber: cfeck.

One more ping. Kate 19.08 freeze is nearing.

cullmann requested changes to this revision.Jul 3 2019, 6:16 PM

I am not sure that this is correct.
Now, always the windows are restored, in-dependent of the config.
I doubt that this is good.

This revision now requires changes to proceed.Jul 3 2019, 6:16 PM

KateSessionManager::saveSessionTo(), only saves the window configuration, when the same condition is true:
(KConfigGroup(KSharedConfig::openConfig(), "General")).readEntry("Restore Window Configuration", true)

KateMainWindow::readProperties() mainly does three things:
KateApp::self()->pluginManager()->enableAllPluginsGUI();
m_fileOpenRecent->loadEntries(KConfigGroup(config.config(), "Recent Files"));
m_viewManager->restoreViewConfiguration(config);

IIUC, nothing to do with KMainWindow configuration per se: https://api.kde.org/frameworks/kxmlgui/html/classKMainWindow.html

That might be true, but you remove the

if (c.readEntry("Restore Window Configuration", true)) {

that guards the whole loop that does e.g. the mainwindow creation.

OK. So "window configuration" here means, the window size, state, and the view properties e.g. splitters... etc. The name threw me off (I confused it with KMainWindow window configuration), and the fact that saveSessionTo() always executes KateMainWindow::saveProperties() regardless of the config setting.

So saveSesssionTo() should only execute saveProperties() if "saveWindowConfig" is true; if you agree with that reasoning, I'll open a separate review request to keep the commits atomic.

Hmm, I don't understand your proposed fix, sorry.

I think we shall just independent of the flag restore the recently opened files and perhaps move that code out of readProperties/saveProperties.

Hmm, I don't understand your proposed fix, sorry.

What I meant is, before fixing this issue, first saveSessionTo() should be modified to only execute saveProperties() if "include window configuration" is enabled, right now saveProperties() is always executed. (I should have just created the review request, looking at actual code is sometimes easier than trying to describe it).

I think we shall just independent of the flag restore the recently opened files and perhaps move that code out of readProperties/saveProperties.

Yes, I had the same idea about splitting the recently opened files code out of read/saveProperties.

for: What I meant is, before fixing this issue, first saveSessionTo() should be modified to only execute saveProperties() if "include window configuration" is enabled, right now saveProperties() is always executed. (I should have just created the review request, looking at actual code is sometimes easier than trying to describe it).

Hmm, I don't think we need to fix this. As it is now, you can later alter the setting and it will use the last stored config, see no harm in this.

OK, fair point.

ahmadsamir updated this revision to Diff 61336.Jul 8 2019, 1:01 PM
ahmadsamir retitled this revision from When loading a session always restore the open recent list to When loading a session always load the "open recent" list.
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir edited the test plan for this revision. (Show Details)

Reworked diff.

First: I think the save/loadOpenRecent makes sense.
But I would call them in both read and saveProperties. Then one doesn't need to touch the code paths for "restore with window config" at all.
For the the code in the session manager: why not just a loop over the existing main windows for the else case that das a loadOpenRecent?

cullmann requested changes to this revision.Jul 8 2019, 5:21 PM
This revision now requires changes to proceed.Jul 8 2019, 5:21 PM
ahmadsamir added a comment.EditedJul 8 2019, 6:58 PM

First: I think the save/loadOpenRecent makes sense.
But I would call them in both read and saveProperties. Then one doesn't need to touch the code paths for "restore with window config" at all.

Of course.

For the the code in the session manager: why not just a loop over the existing main windows for the else case that das a loadOpenRecent?

When opening kate with -n or -s, loadSession() may be called before any KateMainWindow objects have been created, so we have to check and create one before calling loadOpenRecent. I'll change the patch to loop over the existing main windows if there are any.

For: When opening kate with -n or -s, loadSession() may be called before any KateMainWindow objects have been created, so we have to check and create one before calling loadOpenRecent. I'll change the patch to loop over the existing main windows if there are any.

Then just call the loadOpenRecent in the constructor, too, but I don't think we want to start to create windows there.

Hmm, the constructor actually already does:

m_fileOpenRecent->loadEntries(KConfigGroup(sconfig, "Recent Files"));

Hmm, the constructor actually already does:

m_fileOpenRecent->loadEntries(KConfigGroup(sconfig, "Recent Files"));

That uses katerc, not a .katesession file.

FWIW, in one of the iterations working on this diff, I tested by creating a signal, sessionLoaded, that's emitted at the end of loadSession(), the problem was a KateMainWindow hadn't been created yet, so it never caught it in time... it would work when opening/loading a session from the Session menu, but not when launching kate.

I think the easiest solution is to create a new main window if needed in loadSession, startupKate() will do it once control returns to it anyway.

Ok, I see. Then I have no issues with that part of the patch.

ahmadsamir updated this revision to Diff 61429.Jul 9 2019, 4:53 PM
  • Make readProperties() call loadOpenRecent()
  • If there are any kate main windows, loop over them

Hi,

looks nearly ok, but in this loop:

for (int i = 0; i < windowsCount ; ++i) {
          // if there are no main windows, create one to call loadOpenRecent()
          if(i == windowsCount) {
              KateMainWindow *w = KateApp::self()->newMainWindow();
              if (w !=nullptr) {
                  w->loadOpenRecent(cfg);
              }
          } else {
              KateApp::self()->mainWindow(i)->loadOpenRecent(cfg);
          }
      }

How shall there the if(i == windowsCount) {
part be ever feasible?

cullmann accepted this revision.Jul 13 2019, 3:46 PM

I will take care of the last issue and apply this then.

This revision is now accepted and ready to land.Jul 13 2019, 3:46 PM
This revision was automatically updated to reflect the committed changes.

Hi,

looks nearly ok, but in this loop:

for (int i = 0; i < windowsCount ; ++i) {
          // if there are no main windows, create one to call loadOpenRecent()
          if(i == windowsCount) {
              KateMainWindow *w = KateApp::self()->newMainWindow();
              if (w !=nullptr) {
                  w->loadOpenRecent(cfg);
              }
          } else {
              KateApp::self()->mainWindow(i)->loadOpenRecent(cfg);
          }
      }

How shall there the if(i == windowsCount) {
part be ever feasible?

If there are no main windows, then windowsCount is 0 and i is 0 in the first iteration.

IIUC, a similar logic is used in the for loop before this one:

for (int i = 0; i < wCount; ++i) {
    if (i >= KateApp::self()->mainWindowsCount()) {
        KateApp::self()->newMainWindow(cfg, QStringLiteral("MainWindow%1").arg(i));
    } else {
        KateApp::self()->mainWindow(i)->readProperties(KConfigGroup(cfg, QStringLiteral("MainWindow%1").arg(i)));
    }

    KateApp::self()->mainWindow(i)->restoreWindowConfig(KConfigGroup(cfg, QStringLiteral("MainWindow%1 Settings").arg(i)));
}

If windowsCount is 0, you never will be in the first iteration.

The other code doesn't use the same max value for the loop and the check.

If windowsCount is 0, you never will be in the first iteration.

The other code doesn't use the same max value for the loop and the check.

Right.