Add Reset Zoom Level action inside View menu
ClosedPublic

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

Details

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
There are a very large number of changes, so older changes are hidden. Show Older Changes
shubham marked 2 inline comments as done.Jul 13 2019, 5:56 PM

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.Jul 25 2019, 3:23 PM
ngraham edited reviewers, added: shubham; removed: ngraham.

Sure, I'd be happy to.

ngraham updated this revision to Diff 62548.Jul 25 2019, 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)Jul 25 2019, 3:43 PM
ngraham edited the test plan for this revision. (Show Details)
cfeck added a subscriber: cfeck.Jul 25 2019, 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.Aug 3 2019, 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.Aug 3 2019, 11:50 AM
This comment was removed by elvisangelaccio.
ngraham updated this revision to Diff 63764.Aug 14 2019, 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.Aug 14 2019, 8:55 PM

Undo unnecessary CMake changes

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

Clean up remaining whitespace issues

OK, it's ready again now. :)

broulik added inline comments.
src/dolphinui.rc
2

Careful, this clashes with the recent toolbar shuffling

src/views/dolphinview.cpp
1351

Q_UNREACHABLE(); Why bother having a default case, though?

1355

const

ngraham updated this revision to Diff 64868.Aug 28 2019, 4:39 PM
  • Rebase
  • Address review comments
ngraham marked 3 inline comments as done.Aug 28 2019, 4:39 PM

Am I wrong or the new action doesn't show up in the View menu on the menubar?

ngraham added a comment.EditedSep 1 2019, 3:31 PM

That's strange, it worked before (see the screenshot), but now I don't see it there either. :/ Will investigate and fix.

ngraham updated this revision to Diff 65465.Sep 5 2019, 5:43 PM

Fix action not getting added to menu

ngraham updated this revision to Diff 65468.Sep 5 2019, 6:21 PM

Fix it for realsies

This revision is now accepted and ready to land.Sep 10 2019, 10:05 AM
This revision was automatically updated to reflect the committed changes.