Add Reset Zoom Level action inside View menu
Needs ReviewPublic

Authored by ngraham on Jul 13 2019, 5:05 PM.

Details

Reviewers
elvisangelaccio
shubham
Group Reviewers
Dolphin
Summary

FEATURE: 409591
FIXED-IN: 19.12.0

Test Plan

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D22444
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15118
Build 15136: arc lint + arc unit
shubham created this revision.Jul 13 2019, 5:05 PM
Restricted Application added projects: Dolphin, Documentation. · View Herald TranscriptJul 13 2019, 5:05 PM
Restricted Application added subscribers: kde-doc-english, kfm-devel. · View Herald Transcript
shubham requested review of this revision.Jul 13 2019, 5:05 PM
shubham added a subscriber: Dolphin.
Restricted Application removed a subscriber: Dolphin. · View Herald TranscriptJul 13 2019, 5:07 PM
shubham updated this revision to Diff 61709.Jul 13 2019, 5:09 PM

Add / atend of action name in .rc

shubham updated this revision to Diff 61711.Jul 13 2019, 5:11 PM

Correct documentation

ngraham requested changes to this revision.Jul 13 2019, 5:13 PM

In general +1 on the concept, but needs revision before it can go in.

src/dolphinui.rc
118

Put it in between Zoom in and Zoom out, as it is in Okular and Konsole.

src/views/dolphinviewactionhandler.cpp
205

Name should be "Reset zoom level" or "Zoom to default size"

467

That seems like a fairly arbitrary value to reset it to. You should read the default size and zoom to that instead. After all, that's what the WhatsThis text you wrote says it will do. :)

This revision now requires changes to proceed.Jul 13 2019, 5:13 PM
shubham updated this revision to Diff 61715.Jul 13 2019, 5:56 PM
shubham marked 2 inline comments as done.

Set zoom to 5px

shubham added inline comments.Jul 13 2019, 5:58 PM
src/views/dolphinviewactionhandler.cpp
467

There is nothing like "default". zoomlevelinfo.cpp also have only min and max values for levels. I have choosen 5px as it looks perfect.

There is nothing like "default"

Sure there is:

shubham updated this revision to Diff 61745.Jul 14 2019, 3:33 PM

use default value

shubham retitled this revision from Add zoom reset action to [RFC]: Add zoom reset action.Jul 14 2019, 3:33 PM
shubham added inline comments.
src/views/dolphinviewactionhandler.cpp
483

This is troubling me for some time now. It gives undefined reference linker error. I have already included the corresponding header file.

shubham edited the summary of this revision. (Show Details)Jul 14 2019, 3:35 PM
shubham updated this revision to Diff 61778.EditedJul 15 2019, 7:53 AM

Get default icon size
@ngraham Ping

shubham retitled this revision from [RFC]: Add zoom reset action to Add zoom reset action.Jul 15 2019, 7:54 AM
shubham edited the summary of this revision. (Show Details)

Cool, it works and the code is looking pretty good. Now I think the new action needs to be added to the View menu too, between the zoom in and zoom out menu items. The place where you've added it in dolphinui.rc doesn't actually do that.

src/views/dolphinviewactionhandler.cpp
192

Sorry, I steered you wrong. The text needs to Use Title Case:

"Reset zoom level"

This revision now requires changes to proceed.Jul 15 2019, 8:25 PM
shubham updated this revision to Diff 61839.Jul 16 2019, 6:15 AM
shubham marked an inline comment as done.
shubham edited the summary of this revision. (Show Details)

Add action to View menu

shubham retitled this revision from Add zoom reset action to Add Reset Zoom Level action inside View menu.Jul 16 2019, 6:17 AM
shubham edited the test plan for this revision. (Show Details)

I have added the action in .rc files still I can not see them added in the menu.

Ah, looks like Dolphin conditionally adds the zoom actions to the main menu in dolphinmainwindow.cpp (lines 984-988). That's where you should add the new action. Then I guess you don't need to touch the rc files.

@shubham are you able to make those changes?

@shubham are you able to make those changes?

I am busy right now with GSoC, if you want you can complete it.

ngraham commandeered this revision.Thu, Jul 25, 3:23 PM
ngraham edited reviewers, added: shubham; removed: ngraham.

Sure, I'd be happy to.

ngraham updated this revision to Diff 62548.Thu, Jul 25, 3:39 PM

Add new action to View menu properly

ngraham marked 2 inline comments as done.

@elvisangelaccio This is now ready for review.

ngraham edited the summary of this revision. (Show Details)Thu, Jul 25, 3:43 PM
ngraham edited the test plan for this revision. (Show Details)
cfeck added a subscriber: cfeck.Thu, Jul 25, 3:48 PM

Does our HIG say the order is In/Out/Reset or In/Reset/Out?

The HIG is silent on the matter, but Okular and Konsole put the reset action in the middle, so I think it makes sense to do the same here.

Shouldn't the zoom out shortcut be ctrl-- or is it already occupied?

Shouldn't the zoom out shortcut be ctrl-- or is it already occupied?

Um, it already is. :) The screenshot even shows it. :)

If I am not blind, it shows ctrl+-. :)

You're not blind, but that's how keyboard shortcuts look in KDE software.

Oops, I had a perception that you have to press - twice to zoom out. Maybe it came to me from Windows.

elvisangelaccio requested changes to this revision.Sat, Aug 3, 11:50 AM
src/views/dolphinviewactionhandler.cpp
191

Please remove the spaces before/after QStringLiteral

203

Unrelated change, should go to its own commit. (feel free to push it without review...)

467–485

Sorry but I don't like this solution (i.e. creating a settings tab widget inside the action handler). Please try the following instead:

  1. Create a new method DolphinView::resetZoomLevel().
  2. Call settings->useDefaults(true) where settings is one of IconsModeSettings::self(), CompactModeSettings::self(), or DetailsModeSettings::self() (depending on the current DolphinView::viewMode()).
  3. Get the default zoom value from ViewModeSettings::iconSize().
This revision now requires changes to proceed.Sat, Aug 3, 11:50 AM
This comment was removed by elvisangelaccio.
ngraham updated this revision to Diff 63764.Wed, Aug 14, 8:53 PM
ngraham marked 3 inline comments as done and 2 inline comments as done.

Use a more typical approach

ngraham updated this revision to Diff 63765.Wed, Aug 14, 8:55 PM

Undo unnecessary CMake changes

ngraham updated this revision to Diff 63767.Wed, Aug 14, 9:04 PM

Clean up remaining whitespace issues

OK, it's ready again now. :)