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

OPDS based languages and categories #960

Merged
merged 6 commits into from
Aug 2, 2023
Merged

OPDS based languages and categories #960

merged 6 commits into from
Aug 2, 2023

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Jul 11, 2023

Fixes #934
Requires libkiwix from: kiwix/libkiwix#967

src/contentmanager.cpp Outdated Show resolved Hide resolved
src/contentmanager.cpp Outdated Show resolved Hide resolved
@juuz0 juuz0 force-pushed the opdsLanguages branch 2 times, most recently from 4127b0a to be8351c Compare July 12, 2023 16:25
@juuz0 juuz0 requested a review from mgautierfr July 12, 2023 16:26
@juuz0 juuz0 force-pushed the opdsLanguages branch 2 times, most recently from 92da0dd to 6de6a3a Compare July 23, 2023 13:43
@juuz0 juuz0 mentioned this pull request Jul 23, 2023
@juuz0 juuz0 changed the base branch from main to qtlibrary July 27, 2023 13:11
@juuz0 juuz0 force-pushed the qtlibrary branch 4 times, most recently from 2c2046a to 566902c Compare July 30, 2023 17:08
Base automatically changed from qtlibrary to main August 1, 2023 10:31
@kelson42
Copy link
Collaborator

kelson42 commented Aug 1, 2023

@juuz0 Can you please rebase and update about the status? Are you just missing a new review?

@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 1, 2023

@kelson42 Yes, it's missing a new review. The libkiwix side is merged and done.
@mgautierfr Please!

src/contentmanager.cpp Outdated Show resolved Hide resolved
}
mp_reply = m_networkManager.get(request);
connect(mp_reply, &QNetworkReply::finished, this, &OpdsRequestManager::receiveContent);
auto mp_reply = m_networkManager.get(request);
Copy link
Member

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 ?

Copy link
Collaborator Author

@juuz0 juuz0 Aug 2, 2023

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

  1. Have 2 functions:
    opdsResponseFromPath(path) (which calls the next one with an empty query)
    opdsResponseFromPath(path, query)

  2. Have a default empty query parameter
    opdsResponseFromPath(path, query = "")

I went with the 2nd one for now

Copy link
Member

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.

src/kiwixapp.h Outdated Show resolved Hide resolved
src/contentmanager.cpp Show resolved Hide resolved
Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

LGTM

⚠️ Please rebase-fixup before merging ⚠️

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
@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 2, 2023

Rebased fixups

@kelson42 kelson42 merged commit 7c4cd1d into main Aug 2, 2023
4 checks passed
@kelson42 kelson42 deleted the opdsLanguages branch August 2, 2023 12:50
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 this pull request may close these issues.

Don't use a static list of language/categories in the catalog filtering.
3 participants