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

Zim format detection does not work in 2.12.1 and 2.12.2 #2316

Closed
4 tasks done
Self-Perfection opened this issue Jun 1, 2024 · 12 comments · Fixed by #2307
Closed
4 tasks done

Zim format detection does not work in 2.12.1 and 2.12.2 #2316

Self-Perfection opened this issue Jun 1, 2024 · 12 comments · Fixed by #2307
Assignees
Labels
Milestone

Comments

@Self-Perfection
Copy link
Contributor

⚠️ This issue respects the following points: ⚠️

  • This is a bug. Not a question or feature request.
  • The topic is not already reported at Issues. (I've searched it).
  • Markor is up to date. See Releases for the latest version. Updates are available from F-Droid and GitHub.
  • The bug is still present in the latest development version (git master). (Please download and try the test version of Markor, named Marder. Don't worry; Markor and Marder appear as completely separate applications. You can install both side-by-side, and Markor settings are not touched. In case the issue is resolved there, you don't need to create a bug report. The change will be part of the next Markor update.)

Description

Zim files are handled as plain text files. There is no format parsing, zim wiki headers are not hidden, tags are not handled.

Steps to reproduce

  1. Open zim wiki file
  2. It is shown as plain text

Information

Android version: irrelevant
Device: irrelevant
App Version: 2.12.2

Source

F-Droid

Format / File type

Wikitext, Zim

Additional info / Log

-
@harshad1
Copy link
Collaborator

harshad1 commented Jun 2, 2024

Also solved in #2088 #2307

Edit. Sorry, wrong pr linked

@Self-Perfection
Copy link
Contributor Author

Also solved in #2088

That PR is closed a year ago but the issue is a recent regression. I suppose it was introduced about a month or two ago

@fredericjacob
Copy link
Contributor

fredericjacob commented Jun 3, 2024

I can confirm that with the latest version Zim files are not detected as Zim any more, just as plaintext.
Zim pages are in fact *.txt, but they start with the "header" lines e.g.

Content-Type: text/x-zim-wiki Wiki-Format: zim 0.6 Creation-Date: 2019-01-14T23:41:51+01:00

Probably this file content check for *.txt was broken somehow, so Markor just treats them as plain text files due to the ending.

If #2307 fixes the issue already, nice! :)

@fredericjacob
Copy link
Contributor

fredericjacob commented Jun 3, 2024

@harshad1 I just took a look into #2307 , and if I didn't overlook something, I think it won't fix the issue, unfortunately.
I analyzed the problem a bit (just looked at the Github code via Octodroid on the smartphone, so I didn't test anything), and I think I found the reason for the problem.

It seems that this commit introduced the bug:
f35383a

In Document.java, you removed the if-elseif-Statements with a loop iteration over the newly introduced FormatRegistry.FORMATS:

Screenshot_20240604-000918_1
Screenshot_20240604-001103_1

The problem here is: Just using the lower-case filename fnlower in isFileOutOfThisFormat(fnlower) in a generic way is not feasible. In the original code most calls used fnlower as the argument, but Zim, Embedbinary and Orgmode used getPath(): isFileOutOfThisFormat(getPath()). And using the full path of the file is important, because Markor needs it to open the file, look into the content and decide if it contains a Zim header (don't know about the other two formats, but probably they need to be checked somehow too - so there could be similar bugs).
Moreover, the order of the file format checks is important: Originally, plaintext format was only used as fallback if no other format matches before. With the changed code, plaintext is at position 2 and Zim at position 4, so Zim files (ending *.txt just as plaintext) don't have any chance of being matched as Zim.
But like I said, it is necessary to also look into the file contents in addition to checking the line ending to decide if a file is Zim or just an "ordinary" plaintext file, so changing the order is not enough.

@harshad1
Copy link
Collaborator

harshad1 commented Jun 3, 2024

Ah. You're right. I will fix this today

@harshad1
Copy link
Collaborator

harshad1 commented Jun 4, 2024

I have this fixed and pushed up in #2307

Please test.

@fredericjacob
Copy link
Contributor

@harshad1 Ah I saw that you already changed the order of the formats in a previous commit. 👍
Could work now with the change to the file path instead of just the lower case filename.

I just have my smartphone right now and cannot build the APK for testing, however, and I don't even have an Android dev environment set up on my computer at this time... ^^
But there's a valid Zim page under samples/Zim-Sample-Notebook/Root_Page.txt, so you could check if highlighting works if you open this file.

Maybe also @Self-Perfection could do a test if it works now.

Additionally I found a suspicious line in WikitextTextConverter - but this seems to have been there before in a similar form, so probably not exactly related to the issue here:
return Arrays.asList(new String[]{".wikitext"}).contains(ext);
I don't know what this ".wikitext" string could be - if it is supposed to be a file ending, then it's wrong, there is no such file ending as ".wikitext" in Zim, just ".txt". (The term "Wikitext" in general is not related to Zim in any way and not a good naming for the specific format IMO, but that's a different story.)

@Self-Perfection
Copy link
Contributor Author

Sorry, I can not test. Compiling from source code is beyond my capabilities now.

@gsantner
Copy link
Owner

gsantner commented Jun 5, 2024 via email

@Self-Perfection
Copy link
Contributor Author

testbuilsa of prs are at the github actions tab.

👍 Wow. Much CI. Such technology.

So I tested apk from #2307 and it indeed restored zim format handling

@fredericjacob
Copy link
Contributor

Nice to know that the CI pipelines includes test apks. 👍
I tried it out as well and can confirm: the zim format detection works.

@harshad1
Copy link
Collaborator

Marking this done as the fix is in #2307

@gsantner gsantner linked a pull request Jul 20, 2024 that will close this issue
@gsantner gsantner added the bug label Jul 20, 2024
@gsantner gsantner added this to the Markor v2.13 milestone Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants