-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
OPDS based languages and categories #960
Conversation
4127b0a
to
be8351c
Compare
92da0dd
to
6de6a3a
Compare
2c2046a
to
566902c
Compare
@juuz0 Can you please rebase and update about the status? Are you just missing a new review? |
@kelson42 Yes, it's missing a new review. The libkiwix side is merged and done. |
src/opdsrequestmanager.cpp
Outdated
} | ||
mp_reply = m_networkManager.get(request); | ||
connect(mp_reply, &QNetworkReply::finished, this, &OpdsRequestManager::receiveContent); | ||
auto mp_reply = m_networkManager.get(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we set the builded url (before this line) with opdsResponseFromPath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im divided on whats the better way here
-
Have 2 functions:
opdsResponseFromPath(path)
(which calls the next one with an empty query)
opdsResponseFromPath(path, query)
-
Have a default empty query parameter
opdsResponseFromPath(path, query = "")
I went with the 2nd one for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the second is better.
The first option is useful when you have a function foo(bar)
existing and you want to add a optional parameter without breaking the ABI. If you add a parameter with a default value, you change the signature of the function and so you break the ABI. By adding a second method foo(bar, baz)
and changing the implementation of foo(bar)
, you don't break the ABI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Done necessary refactoring in OpdsRequestManager to allow different types of requests. Added functions to provide languages and categories from an OPDS feed.
ContentManager now keeps a list of current categories and languages. Added functions to change this based on local or remote libary
The category values are not static after this change. If local library is displayed, we get the categories from local library If remote library is displayed, we send a request to /catalog/v2/categories
Similar to categories, Languages in the selector are now shown based on the library.
Added checks to not let unnecessary requests to remote library happen.
Removed references to static_content, now that it has been replaced by libkiwix API
Rebased fixups |
Fixes #934
Requires libkiwix from: kiwix/libkiwix#967