Prevent saving Recent Files and LastTargetDir if history is disabled
ClosedPublic

Authored by muhlenpfordt on Mar 12 2018, 1:03 PM.

Details

Summary

Disabling history in SettingsConfigureAdvancedHistory
does not prevent showing and populating FileOpen Recent.
The last target directory for Copy/Move/Link To dialog does not check
for history option too.
This patch only saves/restores file history if the option is enabled
and hides the menu entry if disabled.

BUG: 332853
BUG: 391527

Depends on D11280

Test Plan
  • Enable/disable history option
  • Open some images/folders in Gwenview
  • Copy/Move/Link To some images
  • Check if history is (not) saved/restored according to option value

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.Mar 12 2018, 1:03 PM
muhlenpfordt created this revision.

There's one section in the config file that remains after disabling the history. It's from the QFileDialog/KFileWidget (e.g. Copy/Move/Link dialog).

[KFileDialog Settings]
Recent Files...
Recent URLs...

I found no option to disable this. But probably this should be a KDE thing and maybe affected by System SettingsActivitiesPrivacy.

app/mainwindow.cpp
1490

Always saving the entries here makes sure the old list is removed from the config file after disabling the history.

lib/gwenviewconfig.kcfg
58–60 ↗(On Diff #29328)

Maybe this part part should go to another patch or needs some basic discussion?

rkflx added a subscriber: rkflx.

Two bugs in one patch, nice ;) Some minor comment, otherwise works and looks great.

For KFileDialog and (more importantly) other problems I found when looking at that part of Gwenview I opened T8194: Fix remaining issues with clearing recent files and folders.

lib/contextmanager.cpp
313

Any reason in particular you remove this?

314–316

One more comment about the formatting. I have often seen it like this:

d->mTargetDirUrl = GwenviewConfig::historyEnabled() ? url
                                                    : QUrl();
lib/gwenviewconfig.kcfg
58–60 ↗(On Diff #29328)

Yup, please split out this part and add CCBUG.

I suggested this back in November (how time flies…) in D8785#inline-39402 and the code was kinda cool, but thinking about it now, it's probably making more sense for the importer, and here we can simply start from the current directory the user is in.

Or do you think otherwise and want to keep it? I'm always open to good arguments…

muhlenpfordt edited the summary of this revision. (Show Details)

Moved default for LastTargetDir part to other diff

muhlenpfordt added inline comments.Mar 13 2018, 10:57 AM
lib/contextmanager.cpp
313

Using an empty (=invalid) Url is ok now if we have no default location and the dialog starts in the current folder. But why not check what we really expect here... ;)
(Moved to other diff.)

314–316

Looks better for this short one. :)

lib/gwenviewconfig.kcfg
58–60 ↗(On Diff #29328)

Meanwhile I think starting in the current folder is the most useful default location. I'm in favour of removing PicturesLocation.

rkflx accepted this revision.Mar 13 2018, 8:30 PM
This revision is now accepted and ready to land.Mar 13 2018, 8:30 PM
This revision was automatically updated to reflect the committed changes.