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.

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

Move ZimHostActivity to app module #1600

Closed
macgills opened this issue Nov 21, 2019 · 8 comments · Fixed by #1601
Closed

Move ZimHostActivity to app module #1600

macgills opened this issue Nov 21, 2019 · 8 comments · Fixed by #1601
Assignees
Milestone

Comments

@macgills
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Custom apps should not be allowed to host books

Describe the solution you'd like
Move zimHosting to app module

@macgills macgills self-assigned this Nov 21, 2019
@macgills macgills added this to the 3.1 milestone Nov 21, 2019
@kelson42
Copy link
Collaborator

kelson42 commented Nov 21, 2019

@macgills What is exactly the current problem of having this feature in the custom app? Could we discuss that a bit first?

@macgills
Copy link
Contributor Author

They could only host one file and the feature would have to be altered to support obb files.

@kelson42
Copy link
Collaborator

@macgills What exactly the problem with the obb, really surprised it does not work ou of the box?

@macgills
Copy link
Contributor Author

Because they are not zim files. We do not add them the same way. Zim files get added to a database when we download/detect them and that database is what powers many screens that allow for viewing zim files.
It is certainly possible to implement but it doesn't seem to me like a feature we want in custom apps as a custom app is simply a browser for one book, in my current understanding

@kelson42
Copy link
Collaborator

Even if we have only one book, I would prefer to have the wifi hotspot working for it. This is not a priority.

Considering that the obb is only a zim file with another file extension, I don't believe/understand we use the libkiwix differently. I guess the current incompatibility comes from Java/Kotlin code, but here again it is not fully clear why this use case is different from loading a random ZIM file.

Therefore, I believe that the fix could happen by rethinking a few things in the Java/Kotkin code and the feature should as a consequence stay in core.

I would recommend to mask only the feature for now in custom apps and open a ticket to properly analyse/implement the feature for custom apps as as well.

@macgills Does that make sense to you?

@macgills
Copy link
Contributor Author

macgills commented Nov 21, 2019

We would have to entirely rework the UI, it would look less than stellar with only one book and no way to add more, from its current name "Host Books", the selections ui, all dead space

@kelson42
Copy link
Collaborator

@macgills Thx for confirming this is only a UI Java/Kotlin issue. And "yes" there is only one book in the library. I'm not sure to fully understand the problem here. This UI needs anyway to be improved (a few tickets are already open), so this is probably one of the things to improve. Please just open the ticket to reintroduce the feature, if you remove it for now in custom apps. How you deal with the temporary solution, does not really matter to me, just hiding the menu item seems easier to me, but if you are not afraid to re-introduce the feature to core later, no problem to me :)

@macgills
Copy link
Contributor Author

Well it raises a lot of questions for the user.
"Why can I deselect the book"
"why does the ui talk about multiple books"
etc

The screen would be clearer with an icon of the custom app and a button "start hosting" or something equally simplistic.

I had done 99% of the work by your first comment so I have opened a PR that moves it to app.

If we want a different UI for custom then in the future I will have to move a few classes back.

If we want to reuse the same UI I will have to move the entire thing back, more or less.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants