Readme on old features and rewording of title vs direntry #241

mossroy · 2017-05-28T10:29:33Z

Getting rid of this "Title" concept modifies a lot of code.
@sharun-s : is that what you expected in #218 ?

As suggested in #218

sharun-s

Mostly related to titleName -> title, and dirEntry.name() to dirEntry.title.

sharun-s · 2017-05-28T10:54:22Z

www/js/lib/util.js

+ function removeDuplicateTitlesInDirEntryArray(array) {
+ array.sort(function(dirEntryA, dirEntryB) {
+ if (dirEntryA.title < dirEntryB.title) return -1;
+ if (dirEntryA.title > dirEntryB.title) return 1;


This dirEntry.title property is also accessed via dirEntry.name and dirEntry.getReadableName. Maybe this name and getReadableName can be removed so that everywhere there is just one way title is accessed.

I agree. It was probably meaningful with Evopedia archives, but is not any more.

sharun-s · 2017-05-28T11:05:28Z

www/js/lib/zimArchive.js

- callback(title.name(), utf8.parse(data));
+ ZIMArchive.prototype.readArticle = function(dirEntry, callback) {
+ return dirEntry.readData().then(function(data) {
+ callback(dirEntry.name(), utf8.parse(data));


This can be just dirEntry.title instead of .name()

sharun-s · 2017-05-28T11:06:12Z

www/js/lib/zimArchive.js

- callback(title.name(), data);
+ ZIMArchive.prototype.readBinaryFile = function(dirEntry, callback) {
+ return dirEntry.readData().then(function(data) {
+ callback(dirEntry.name(), data);


Also remembered this was a weird flow while debugging readArticle.
zimdirent.readData just calls zimfile.blob. this readData function seemed an extra step.

Not quite : it hides the concept of cluster

sharun-s · 2017-05-28T11:08:09Z

www/js/lib/zimArchive.js

@@ -270,7 +254,7 @@ define(['zimfile', 'zimDirEntry', 'util', 'utf8'],
 * @param {String} titleName
 * @return {Promise} resolving to the title object or null if not found.
 */
- ZIMArchive.prototype.getTitleByName = function(titleName) {
+ ZIMArchive.prototype.getDirEntryByTitleName = function(titleName) {


Here too Name can be removed

titleName can be changed to title everywhere I think

sharun-s · 2017-05-28T11:27:02Z

README.md

This would be I think more readable and stick in memory of devs as a simple list rather than a para.

Useful Features Unavailable in Current Build:

Evopedia archive support

Find Articles Nearby (Geo based search)

Firefox OS port

Phonegap/Cordova port (incomplete)
Please consult Git History for reference.

sharun-s · 2017-05-28T11:29:55Z

tests/tests.js

- assert.ok(title !== null, "Title found");
- assert.ok(!title.isRedirect(), "Title is not a redirect.");
- assert.equal(title.name(), "Night Time Is the Right Time", "Correct redirected title name.");
+ localZimArchive.getDirEntryByTitleName("A/(The_Night_Time_Is)_The_Right_Time.html").then(function(dirEntry) {


TitleName ->Title
.name() -> .title

sharun-s · 2017-05-28T11:36:53Z

www/js/app.js

 messagePort.postMessage({'action': 'giveContent', 'titleName' : titleName, 'content': content});
 console.log("content sent to ServiceWorker");
 });
 }
 };
- selectedArchive.getTitleByName(titleName).then(readFile).fail(function() {
+ selectedArchive.getDirEntryByTitleName(titleName).then(readFile).fail(function() {


titleName to title

Lots of places in this file where above change applies.
also .name() to .title

sharun-s · 2017-05-28T12:03:08Z

www/js/lib/zimfile.js

@@ -20,7 +20,7 @@
 * along with Kiwix (file LICENSE-GPLv3.txt). If not, see <http://www.gnu.org/licenses/>
 */
 'use strict';
-define(['xzdec_wrapper', 'util', 'utf8', 'q'], function(xz, util, utf8, Q) {
+define(['xzdec_wrapper', 'util', 'utf8', 'q', 'zimDirEntry'], function(xz, util, utf8, Q, zimDirEntry) {


Not sure how to add a comment on the "blob" function. So am adding here.
This function took me the longest to figure out. Okay I didn't know what Promises were so that didn't help :)
But even after...I think what was missing to me was how the decompression process works. Just a brief note in the function documentation would be really useful. Like what is chunk size? Relationship to the compressed data read length. To read the 16 bytes to get blob address for example why is chunk size set to 5 KB. Basically some kind of note on how this works would be great. By changing these sizes (data length/chunk size) can reads be done faster?

nextCluster = readInt(clusterOffsets, 8, 8); This nextCluster doesn't seem to get used anywhere

I don't master this code portion (I did not write it myself).
I agree with you that it lacks some comments. And it might be a place where some optimization could take place.
I'll leave it as it is for now.

sharun-s · 2017-05-28T12:13:03Z

www/js/lib/zimArchive.js

- return title.readData().then(function(data) {
- callback(title.name(), utf8.parse(data));
+ ZIMArchive.prototype.readArticle = function(dirEntry, callback) {
+ return dirEntry.readData().then(function(data) {


Is this return here required? The callback handles the end of the read.

True : it's unnecessary

…eName(). To help understanding the code.

mossroy · 2017-05-28T18:06:17Z

I pushed another commit that addresses some of your comments.
I'd like to merge this PR, test and release a version 2.1.0 that would include all the recent commits we made.
Feel free to open other issues if necessary, for a next version

sharun-s · 2017-05-28T20:35:48Z

Ok. Sounds good to me!

mossroy added 3 commits May 27, 2017 23:13

Minor update README to talk about old removed features.

2b7ff70

As suggested in #218

Remove the concept of Title (remaining from Evopedia archive structure).

c091922

As suggested in #218

Remove the zimArchive._dirEntryToTitleObject function to simplify.

e521f9d

mossroy mentioned this pull request May 28, 2017

Simplify/document the technical details to help contributors get into it #218

Closed

sharun-s reviewed May 28, 2017

View reviewed changes

Reword titleName to title, and remove DirEntry.name() and .getReadabl…

25cda11

…eName(). To help understanding the code.

mossroy merged commit a3d3c76 into master May 28, 2017

mossroy deleted the readme_on_old_features_and_rewording_of_title_vs_direntry branch May 28, 2017 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readme on old features and rewording of title vs direntry #241

Readme on old features and rewording of title vs direntry #241

mossroy commented May 28, 2017 •

edited

Loading

sharun-s left a comment

sharun-s May 28, 2017

mossroy May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

mossroy May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

sharun-s May 28, 2017

mossroy May 28, 2017

sharun-s May 28, 2017

mossroy May 28, 2017

mossroy commented May 28, 2017

sharun-s commented May 28, 2017

Readme on old features and rewording of title vs direntry #241

Readme on old features and rewording of title vs direntry #241

Conversation

mossroy commented May 28, 2017 • edited Loading

sharun-s left a 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

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

mossroy commented May 28, 2017

sharun-s commented May 28, 2017

mossroy commented May 28, 2017 •

edited

Loading