Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add option to hide scrollbars for PDFs #1647

Merged
merged 4 commits into from
Jun 30, 2020
Merged

Add option to hide scrollbars for PDFs #1647

merged 4 commits into from
Jun 30, 2020

Conversation

YongJieYongJie
Copy link
Contributor

I added an option to hide/show scrollbars for my own use, and thought I'll share it upstream. If this feature is useful, do let me know how the code could be tidied up before merging in.

Details:

  • Added HideScrollbars to the settings, under FixedPageUI
  • Scrollbars will be hidden / shown for PDFs based on the option
  • Works for fullscreen single page continuous mode, also tested on the modes that are accessible via shortcut keys Ctrl+1 / 2 / 3 / 5 / 6 / 7 / 8 / 0

Demonstration:

toggling-scrollbars-visibility

and with overlap showing key-presses:
toggling-scrollbars-visibility-with-keys-overlay

Previous discussions:

kjk
kjk reviewed Jun 29, 2020
View reviewed changes
src/EditAnnotations.cpp Outdated
@@ -113,7 +113,7 @@ AnnotationType gAnnotsWithColor[] = {
// clang-format on

// in SumatraPDF.cpp
extern void WindowInfoRerender(WindowInfo*);
extern void WindowInfoRerender(WindowInfo*, bool = FALSE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using bool, use true / false.
Also, name the bool argument.
The rule is: if the argument is self-explanatory (like WindowInfo*), then we might

kjk
kjk requested changes Jun 29, 2020
View reviewed changes
Copy link
Member

@kjk kjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good.

Could you also add context menu `Show Scrollbars" to menuDefContext (like "Show Bookmarks" etc.).
See Menu.cpp and Commands.cpp.

src/EditAnnotations.cpp Outdated
@@ -113,7 +113,7 @@ AnnotationType gAnnotsWithColor[] = {
// clang-format on

// in SumatraPDF.cpp
extern void WindowInfoRerender(WindowInfo*);
extern void WindowInfoRerender(WindowInfo*, bool = FALSE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using bool, use true / false.
Also, name the bool argument.
The rule is: if the argument is self-explanatory (like WindowInfo*), then we might skip naming it. Otherwise, it should be named.

src/DisplayModel.h Outdated
@@ -135,6 +135,8 @@ struct DisplayModel : public Controller {
void Relayout(float zoomVirtual, int rotation);

Rect GetViewPort() const;
bool hScrollbarShown() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more in line with the rest of the code, this should be IsHScrollbarVisible()

src/Canvas.cpp Outdated
@@ -1312,7 +1330,7 @@ static void OnTimer(WindowInfo* win, HWND hwnd, WPARAM timerId) {
case REPAINT_TIMER_ID:
win->delayedRepaintTimer = 0;
KillTimer(hwnd, REPAINT_TIMER_ID);
win->RedrawAll();
win->RedrawAllIncludingNonClient(TRUE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be true because it's bool

src/SettingsStructs.h
@@ -44,6 +44,8 @@ struct FixedPageUI {
Vec<COLORREF>* gradientColors;
// if true, TextColor and BackgroundColor will be temporarily swapped
bool invertColors;
// if true, hides the scrollbars but retain ability to scroll
bool hideScrollbars;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See top of the file. The source of this is in gen_settings_structs.go. So add it there and then run doit.bat -gen-structs to regenerate this file. Requires go to be installed.

@YongJieYongJie
Copy link
Contributor Author

Looks mostly good.

Could you also add context menu `Show Scrollbars" to menuDefContext (like "Show Bookmarks" etc.).
See Menu.cpp and Commands.cpp.

Sure, let me look into that.

@GitHubRulesOK
Copy link
Collaborator

Looks mostly good.
Could you also add context menu `Show Scrollbars" to menuDefContext (like "Show Bookmarks" etc.).
See Menu.cpp and Commands.cpp.
Is looking good for those wanting ScrollBarOff, is there a significant problem in having a similar ScrollBarOn for the vociferous majority that wish to have similarity to Acrobat Always On

@YongJieYongJie
Refactor code pursuant to review
956fe3d 
 - Ensure consistent usage of lowercase `true` and `false`
 - Update user settings via do/gen_settings_structs.go
 - General renaming for consistency
@YongJieYongJie
Add "Show Scrollbars" to view and context menus
f43d896
@kjk kjk merged commit ce7a33c into sumatrapdfreader:master Jun 30, 2020
@kjk
Copy link
Member

kjk commented Jun 30, 2020

Thanks!

@brontosaurusrex
Copy link

Is there a release planed with option to hide scrollbars?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjk kjk kjk requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@YongJieYongJie @GitHubRulesOK @kjk @brontosaurusrex