Skip to content
This repository has been archived by the owner on Nov 26, 2020. It is now read-only.

Map scroll directions to actual segment indices in fonts menu #238

Merged
merged 1 commit into from
May 29, 2017

Conversation

ianyh
Copy link
Contributor

@ianyh ianyh commented May 11, 2017

This is intended to fix the crash in #215. The only options in the menu are vertical and horizontal, so the UI excludes the horizontalWithVerticalContent option, but that option seems to be only configurable programmatically and is incompatible with letting the user select a scroll direction.

@@ -243,7 +243,12 @@ class FolioReaderFontsMenu: UIViewController, SMSegmentViewDelegate, UIGestureRe
scrollDirection = readerConfig.scrollDirection
}

layoutDirection.selectSegmentAtIndex(scrollDirection?.rawValue ?? 0)
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that this is the cause of the crash? Because it fallback to 0 if is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not nil. It's 3.

Copy link
Member

Choose a reason for hiding this comment

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

Have you tried on PR #237 because I am currently reviewing that and this is not happening, how do I reproduce that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't crash if you have different defaults already set for configuration. See this line:

https://github.com/FolioReader/FolioReaderKit/pull/237/files#diff-7d83fdc08c25740f0dc3bab59bd7c2bbL72

It sets the default value to defaultVertical. In this line it takes the default value if nothing has changed it:

https://github.com/FolioReader/FolioReaderKit/pull/237/files#diff-21569209c801652e20649d5072f824d0R255

And then the conditional in this line does not get triggered:

https://github.com/FolioReader/FolioReaderKit/pull/237/files#diff-21569209c801652e20649d5072f824d0R255

Which means that when you reach this line:

https://github.com/FolioReader/FolioReaderKit/pull/237/files#diff-21569209c801652e20649d5072f824d0R255

scrollDirection is still defaultVertical, which has a rawValue of 3. There's only two segments so it crashes.

I expect you have some unique state from developing the project that set your current scroll direction to either horizontal or vertical. I think user defaults are maintained across app deletion, so to reproduce you would have to either

  • manually clear out user defaults programmatically, then relaunch to let the defaults get set again, or
  • reset a simulator to start fresh on it

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you are right :)

@@ -243,7 +243,12 @@ class FolioReaderFontsMenu: UIViewController, SMSegmentViewDelegate, UIGestureRe
scrollDirection = readerConfig.scrollDirection
}

layoutDirection.selectSegmentAtIndex(scrollDirection?.rawValue ?? 0)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you are right :)

@hebertialmeida hebertialmeida merged commit 419d5e8 into FolioReader:master May 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants