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

Issue/140 #237

Merged
merged 136 commits into from
May 30, 2017
Merged

Issue/140 #237

merged 136 commits into from
May 30, 2017

Conversation

tschob
Copy link
Member

@tschob tschob commented May 9, 2017

Implements multiple instances support. Solves #140

There is now a third example project which shows two instances in parallel.

@kevindelord
Copy link

Hi @hebertialmeida !

So an internal PR will soon be reviewed and merged. It will fix the remaining issues and bugs.
Could you please try out the app again and found new introduce bugs?

Best!

@kevindelord
Copy link

Hi again @hebertialmeida,

I'm opening another thread for that small question.
When using horizontalWithVerticalContent, once the user jumps to another chapter (from the chapter list) he is not redirected to the top of the page (with the chapter title) but somewhere else.
This behaviour differs from the vertical and horizontal scroll directions.

I am not sure if this is a bug or feature.
I've also made sure that I have not introduced that problem (if it is one).

Best

@hebertialmeida
Copy link
Member

So, the horizontalWithVerticalContent should behave like that.

The bar was not showing because now with `setup` function it add
multiple times the recognizer to the view, now I remove everything
before adding new one
@hebertialmeida
Copy link
Member

Hey, @tschob and @kevindelord I take a general look at it, improved the animation of tabBar and fixed the bug of not showing it.

The problem was that the gestured is added multiple times to the webView, so when you tap it to toggle the bar the number of times the gestures was added...

Overall it LGTM, so I think we can merge that, take a look at my changes and let me know., then I will merge it.

@kevindelord
Copy link

@hebertialmeida I don't fully understand why you changed the readerContainer to an optional:

open weak var readerContainer: FolioReaderContainer?

But otherwise it looks good to me. You can merge :)

return self.readerContainer.readerConfig
fileprivate var folioReader: FolioReader {
guard let readerContainer = readerContainer else { return FolioReader() }
return self.readerContainer!.folioReader

Choose a reason for hiding this comment

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

like if you had kept the non-optional value this would not have been needed.
Plus you should use the value from the guard instead of the self.readerContainer!.folioReader

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, missed that...


/// Called when the application will resign active
open class func applicationWillResignActive() {
FolioReader.shared.saveReaderState()

Choose a reason for hiding this comment

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

Maybe we should keep that for the legacy? I mean when people update the library they could/should get a simple warning.
The function could be empty though, but at least it would still exist :)

Copy link
Member

Choose a reason for hiding this comment

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

As per discussed by Slack, there is no need for this.


/// Called when the application will terminate
open class func applicationWillTerminate() {
FolioReader.shared.saveReaderState()

Choose a reason for hiding this comment

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

Same remark here :)

Copy link
Member

Choose a reason for hiding this comment

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

As per discussed by Slack, there is no need for this.

fileprivate func addObservers() {
removeObservers()
NotificationCenter.default.addObserver(self, selector: #selector(saveReaderState), name: .UIApplicationWillResignActive, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(saveReaderState), name: .UIApplicationWillTerminate, object: nil)

Choose a reason for hiding this comment

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

Are you sure this works fine with the multiple instances mode? Like the correct epub gets saved?

Copy link
Member

Choose a reason for hiding this comment

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

It should because each observer is added to self and it is a different object in memory.


/// Called when the application will resign active
open class func applicationWillResignActive() {
FolioReader.shared.saveReaderState()
Copy link
Member

Choose a reason for hiding this comment

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

As per discussed by Slack, there is no need for this.


/// Called when the application will terminate
open class func applicationWillTerminate() {
FolioReader.shared.saveReaderState()
Copy link
Member

Choose a reason for hiding this comment

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

As per discussed by Slack, there is no need for this.

@hebertialmeida
Copy link
Member

@kevindelord @tschob Thank you guys, this is a huge improvement!!

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.

3 participants