Replace deprecated functions #831

juuz0 · 2022-04-12T07:13:49Z

This replaces the deprecated Reader and Favicon functions from libkiwix

kiwix::Reader functions are converted to use zim::Archive
Favicon functions are converted to use Illustration
Fixes #805

juuz0 · 2022-04-12T07:18:47Z

Removing the reader in suggestionlistworker.cpp should be the last thing to do, to complete this PR. But I'm stuck on how to get suggestions with an archive (wouldn't exposing getSuggestions() from libkiwix internalServer just make it easier? Points to kiwix/libkiwix#740) so would appreciate help on that.

juuz0 · 2022-04-12T14:34:02Z

(There's a crash I just found, fixing that..)

Edit: Done ✔️

mgautierfr · 2022-04-13T09:59:04Z

Removing the reader in suggestionlistworker.cpp should be the last thing to do, to complete this PR. But I'm stuck on how to get suggestions with an archive (wouldn't exposing getSuggestions() from libkiwix internalServer just make it easier? Points to kiwix/libkiwix#740) so would appreciate help on that.

As say the small comment in https://github.com/kiwix/libkiwix/blob/master/src/server/internalServer.cpp#L408-L440, searching in the title list using "normalized" case should be done in libzim.
But it is not a easy task (this is also why we introduced xapian tittle index)

I'm partisan to only use SuggestionSearcher in kiwix-desktop. It already fallback to title search (but without case normalization).
For old zim file (before 2019-08) it would be a regression as we would not do case normalization title search (and so suggestion may be less accurate).
If this is really a issue, we should do it where it must be done, in libzim (but I'm not sure it worth the work)

src/library.cpp

juuz0 · 2022-04-13T18:35:34Z

For old zim file (before 2019-08) it would be a regression as we would not do case normalization title search (and so suggestion may be less accurate).

For a zim like https://sourceforge.net/projects/telugu-dictionary-zim-file/files/ (from 2016), this will lead to no suggestions, won't that be a major regression for old files?

stale · 2022-04-28T01:21:40Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

kelson42 · 2022-05-07T18:43:29Z

From a user perspective it looks good. I don't see DEPRECATED warnings during the compilation too. @mgautierfr Please have a look to the code.

src/contentmanager.cpp

mgautierfr · 2022-05-10T13:25:53Z

src/readinglistbar.cpp

-        std::string mimeType;
-        reader->getFavicon(content, mimeType);
+        }
+        auto illustration = archive->getIllustrationItem(48);


We have to handle the case the archive as no 48x48 illustration (pretty rare but anyway)

How do you handle it?
Currently, I have just left the catch block empty.

It depends of the use case.
We probably want to display nothing (in the library or in the tab icon) if the zim has no illustration. It seems that lefting the catch block empty is enough.
(In the library we display a "broken" image. It would be better to not display an image at all. But it is not the purpose of this PR)

src/suggestionlistworker.cpp

mgautierfr · 2022-05-10T13:39:08Z

src/urlschemehandler.cpp

+zim::Entry getFinalEntry(zim::Entry entry)
+{
+  int loopCounter = 42;
+  auto final_entry = entry;
+  while (final_entry.isRedirect() && loopCounter--) {
+    final_entry = final_entry.getRedirectEntry();
+  }
+  // Prevent infinite loops.
+  if (final_entry.isRedirect()) {
+    throw zim::EntryNotFound("bad entry");
+  }
+  return final_entry;
+}


zim::Entry::getItem(true) follows the redirection (until we have a item) and then return the associated item. We could use it directly instead this intermediate function.

Is getFinalEntry still needed as you now use getItem(true) ?

mgautierfr · 2022-05-10T13:46:55Z

src/webview.cpp

        return;
    }
    std::string favicon, _mimetype;
-    reader->getFavicon(favicon, _mimetype);
+    auto item = archive->getIllustrationItem(48);


We must handle case where archive has not 48x48 illustration

mgautierfr · 2022-05-17T08:24:18Z

src/library.cpp

+    std::shared_ptr<zim::Searcher> searcher;
+    auto archive = getArchive(zimId);
+    if (!searcher) {
+        searcher = std::make_shared<zim::Searcher>(*archive);
+    } else {
+        searcher->addArchive(*archive);
    }
    return searcher;


This structure (if searcher, add archive else create reader) is usefull only when we loop on a set of archives.
But here we have only one archive

Suggested change

std::shared_ptr<zim::Searcher> searcher;

auto archive = getArchive(zimId);

if (!searcher) {

searcher = std::make_shared<zim::Searcher>(*archive);

} else {

searcher->addArchive(*archive);

}

return searcher;

auto archive = getArchive(zimId);

return std::make_shared<zim::Searcher>(*archive);

mgautierfr · 2022-05-17T08:27:59Z

src/urlschemehandler.cpp

+        BlobBuffer* buffer = new BlobBuffer(entry.getItem().getData(0));
+        auto mimeType = QByteArray::fromStdString(entry.getItem(true).getMimetype());


For consistency, it would be better to have either getItem() or getItem(true). But not both.

mgautierfr · 2022-05-17T08:53:42Z

src/urlschemehandler.cpp

+zim::Entry getFinalEntry(zim::Entry entry)
+{
+  int loopCounter = 42;
+  auto final_entry = entry;
+  while (final_entry.isRedirect() && loopCounter--) {
+    final_entry = final_entry.getRedirectEntry();
+  }
+  // Prevent infinite loops.
+  if (final_entry.isRedirect()) {
+    throw zim::EntryNotFound("bad entry");
+  }
+  return final_entry;
+}


Is getFinalEntry still needed as you now use getItem(true) ?

src/contentmanager.cpp

mgautierfr · 2022-05-17T09:20:31Z

src/readinglistbar.cpp

-        std::string mimeType;
-        reader->getFavicon(content, mimeType);
+        }
+        auto illustration = archive->getIllustrationItem(48);


It depends of the use case.
We probably want to display nothing (in the library or in the tab icon) if the zim has no illustration. It seems that lefting the catch block empty is enough.
(In the library we display a "broken" image. It would be better to not display an image at all. But it is not the purpose of this PR)

mgautierfr · 2022-05-17T09:22:23Z

src/contentmanager.cpp

        if (key == "favicon") {
-            auto s = b->getFavicon();
+            auto s = b->getIllustration(48)->getData();


There is not try around this getIllustration

mgautierfr · 2022-05-17T09:31:01Z

src/suggestionlistworker.cpp

+        int suggestionsCount = 15;
+        auto prefix = m_text.toStdString();
+        auto suggestionSearcher = zim::SuggestionSearcher(*archive);
+        if (archive->hasTitleIndex()) {


Why limiting suggestion to when archive has a title index ?
If archive has no title index, suggestion may indeed not be the best (as we search in the title listing) but it is better than nothing.

mgautierfr · 2022-05-17T09:33:51Z

src/suggestionlistworker.cpp

+            auto suggestionSearch = suggestionSearcher.suggest(prefix);
+            const auto suggestions = suggestionSearch.getResults(0, suggestionsCount);
+            for (auto current : suggestions) {
+                kiwix::SuggestionItem suggestion(current.getTitle(), lcAll(current.getTitle()),


Do we really need to use a kiwix::SuggestionItem ?
Could we feed suggestionList, url and urlList directly from current ?

kelson42 · 2022-05-24T05:16:51Z

@juuz0 Any update?

stale · 2022-06-12T23:54:12Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

mgautierfr · 2022-06-16T08:37:18Z

@juuz0 I've added few fixup commit which should fix my remarks.
Please have a look to them before we merge this branch in master.

src/suggestionlistworker.cpp

juuz0 · 2022-06-16T11:55:15Z

@mgautierfr LGTM and works too. Only thing remaining is my comment above

This replaces the deprecated Reader and Favicon functions from libkiwix kiwix::Reader functions are converted to use zim::Archive Favicon functions are converted to use Illustration Fix #805

Commented out unused variable names in function parameters Casted unused variable (void) in for-loop Fixed a signed-unsigned comparison warning

mgautierfr · 2022-06-16T12:26:03Z

Last comment fixed. Rebase-fixup done.

kelson42 requested review from veloman-yunkan and removed request for veloman-yunkan April 12, 2022 13:28

juuz0 force-pushed the removeLibkiwixDeprecations branch from d0d2e38 to 590a649 Compare April 12, 2022 16:18

mgautierfr reviewed Apr 13, 2022

View reviewed changes

src/library.cpp Outdated Show resolved Hide resolved

src/library.cpp Outdated Show resolved Hide resolved

juuz0 force-pushed the removeLibkiwixDeprecations branch from 590a649 to fc3f093 Compare April 13, 2022 18:27

juuz0 marked this pull request as ready for review April 13, 2022 18:30

juuz0 changed the title ~~[WIP] Replace deprecated functions~~ Replace deprecated functions Apr 13, 2022

stale bot added the stale label Apr 28, 2022

juuz0 removed the stale label May 7, 2022

juuz0 force-pushed the removeLibkiwixDeprecations branch from fc3f093 to a3b19e2 Compare May 7, 2022 05:11

juuz0 requested a review from mgautierfr May 7, 2022 05:11

mgautierfr requested changes May 10, 2022

View reviewed changes

juuz0 force-pushed the removeLibkiwixDeprecations branch 2 times, most recently from 68e38af to afd23e1 Compare May 17, 2022 06:48

juuz0 requested a review from mgautierfr May 17, 2022 06:48

mgautierfr requested changes May 17, 2022

View reviewed changes

stale bot added the stale label Jun 12, 2022

mgautierfr force-pushed the removeLibkiwixDeprecations branch from afd23e1 to 6cf07d4 Compare June 16, 2022 08:35

stale bot removed the stale label Jun 16, 2022

juuz0 commented Jun 16, 2022

View reviewed changes

src/suggestionlistworker.cpp Outdated Show resolved Hide resolved

juuz0 added 2 commits June 16, 2022 14:25

Replace deprecated functions

b05daa1

This replaces the deprecated Reader and Favicon functions from libkiwix kiwix::Reader functions are converted to use zim::Archive Favicon functions are converted to use Illustration Fix #805

Remove unused variable warnings

b49ea7c

Commented out unused variable names in function parameters Casted unused variable (void) in for-loop Fixed a signed-unsigned comparison warning

mgautierfr force-pushed the removeLibkiwixDeprecations branch from 6cf07d4 to b49ea7c Compare June 16, 2022 12:25

kiwix-desktop now need libkiwix >= 11.0.0

b6934d7

mgautierfr approved these changes Jun 16, 2022

View reviewed changes

mgautierfr merged commit c3eace1 into master Jun 16, 2022

mgautierfr deleted the removeLibkiwixDeprecations branch June 16, 2022 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated functions #831

Replace deprecated functions #831

juuz0 commented Apr 12, 2022 •

edited by kelson42

Loading

juuz0 commented Apr 12, 2022

juuz0 commented Apr 12, 2022 •

edited

Loading

mgautierfr commented Apr 13, 2022

juuz0 commented Apr 13, 2022

stale bot commented Apr 28, 2022

kelson42 commented May 7, 2022

mgautierfr May 10, 2022

juuz0 May 17, 2022

mgautierfr May 17, 2022

mgautierfr May 10, 2022

mgautierfr May 17, 2022

mgautierfr May 10, 2022

mgautierfr May 17, 2022

mgautierfr May 17, 2022

mgautierfr May 17, 2022

mgautierfr May 17, 2022

mgautierfr May 17, 2022

mgautierfr May 17, 2022

mgautierfr May 17, 2022

kelson42 commented May 24, 2022

stale bot commented Jun 12, 2022

mgautierfr commented Jun 16, 2022

juuz0 commented Jun 16, 2022

mgautierfr commented Jun 16, 2022

		BlobBuffer* buffer = new BlobBuffer(entry.getItem().getData(0));
		auto mimeType = QByteArray::fromStdString(entry.getItem(true).getMimetype());

Replace deprecated functions #831

Replace deprecated functions #831

Conversation

juuz0 commented Apr 12, 2022 • edited by kelson42 Loading

juuz0 commented Apr 12, 2022

juuz0 commented Apr 12, 2022 • edited Loading

mgautierfr commented Apr 13, 2022

juuz0 commented Apr 13, 2022

stale bot commented Apr 28, 2022

kelson42 commented May 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 commented May 24, 2022

stale bot commented Jun 12, 2022

mgautierfr commented Jun 16, 2022

juuz0 commented Jun 16, 2022

mgautierfr commented Jun 16, 2022

juuz0 commented Apr 12, 2022 •

edited by kelson42

Loading

juuz0 commented Apr 12, 2022 •

edited

Loading