Expose txt_preformatted to UI#13392
Conversation
| function ReaderRolling:onCheckDomStyleCoherence() | ||
| if self.ui.document and self.ui.document:isBuiltDomStale() then | ||
| local has_bookmarks_warn_txt = "" | ||
| -- When using an older DOM version, bookmarks may break |
| function ReaderRolling:showReloadConfirmBox(has_bookmarks_warn) | ||
| local has_bookmarks_warn_txt = has_bookmarks_warn | ||
| and _("\nNote that this change in styles may render your bookmarks or highlights no more valid.\nIf some of them do not show anymore, you can just revert the change you just made to have them shown again.\n\n") | ||
| or "" |
There was a problem hiding this comment.
Dunno if this flag could be used anywhere - but this text makes more sense in its original place than here in a supposedly generic showReloadConfirmBox, which could accept any text rather than just a boolean.
| }) | ||
| table.insert(style_table, { | ||
| text_func = function() | ||
| return _("Preformatted text in TXT files") .. (G_reader_settings:has("txt_preformatted") and "" or " ★") |
There was a problem hiding this comment.
What does this star icon really means ?
Below, it looks like G_reader_settings:has("txt_preformatted") can only be 0 or nil - so it indicates only when it is set globally to 0 - and not when I reset it to nil and the global ends up being 1 ?
Feels a bit odd that it would show only when disabled - meaning the default state is on, which has been our historical default, but users don't know our history - and future users would feel that odd.
Do we have that kind of strange behaviour elsewhere?
There was a problem hiding this comment.
As usual, the checkbox shows the status of a setting per document, and the star - the status for new documents.
There was a problem hiding this comment.
But the star won't show if it's enabled (our default) for all document, no?
There was a problem hiding this comment.
By default there is no txt_preformatted in G_reader_settings and the star is shown.
| callback = function() | ||
| self.txt_preformatted = self.txt_preformatted == 1 and 0 or 1 | ||
| self.ui.doc_settings:saveSetting("txt_preformatted", self.txt_preformatted) | ||
| self.ui.rolling:showReloadConfirmBox() |
There was a problem hiding this comment.
Do you need here to explicitely show that ConfirmBox ?
Haven't looked, but how does toggling this affect the DOM/HTML build from the document ?
Does a change in txt_preformatted doesn't by itself cause isBuiltDomStale and implicitely a "style has changed in such a way" warning ?
There was a problem hiding this comment.
Trying to change txt_preformatted on the fly causes segmentation fault.
There was a problem hiding this comment.
So tell us :), and I may look at it.
And at least, mention that in a comment, so we remember it and understand why the ConfirmBox (and so that, later, you may have the joy of killing that comment :))
There was a problem hiding this comment.
For reference:
Thread 1 "luajit" received signal SIGSEGV, Segmentation fault.
0x00007fffe036ade1 in LDOMNameIdMap::findItem (this=this@entry=0x55555716e9f8, name=name@entry=0x5555571b3e60 U"FictionBook") at thirdparty/kpvcrlib/crengine/crengine/src/lstridmap.cpp:223
223 r = lStr_cmp( name, m_by_name[c]->value.c_str() );
(gdb) bt
#0 0x00007fffe036ade1 in LDOMNameIdMap::findItem (this=this@entry=0x55555716e9f8, name=name@entry=0x5555571b3e60 U"FictionBook") at thirdparty/kpvcrlib/crengine/crengine/src/lstridmap.cpp:223
#1 0x00007fffe02e8552 in lxmlDocBase::getElementNameIndex (this=this@entry=0x55555715e710, name=0x5555571b3e60 U"FictionBook") at thirdparty/kpvcrlib/crengine/crengine/src/lvtinydom.cpp:3869
#2 0x00007fffe02fa596 in ldomDocument::createXPointerV1 (this=0x55555715e710, baseNode=<optimized out>, xPointerStr=...) at thirdparty/kpvcrlib/crengine/crengine/src/lvtinydom.cpp:10588
#3 0x00007fffe03233b1 in ldomDocument::createXPointer (this=this@entry=0x55555715e710, baseNode=<optimized out>, xPointerStr=...)
at thirdparty/kpvcrlib/crengine/crengine/src/../include/lvtinydom.h:2789
#4 0x00007fffe02faaf0 in ldomDocument::createXPointer (this=this@entry=0x55555715e710, xPointerStr=...) at thirdparty/kpvcrlib/crengine/crengine/src/lvtinydom.cpp:9532
#5 0x00007fffe0255350 in getPageFromXPointer (L=0x7ffff7cb6380) at build/x86_64-linux-gnu-debug/cmake/koreader/build/cre.cpp:939
#6 0x00005555555b8396 in lj_BC_FUNCC () at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/buildvm_x86.dasc:857
#7 0x00005555555af01c in lua_pcall (L=L@entry=0x7ffff7cb6380, nargs=nargs@entry=1, nresults=-1, errfunc=errfunc@entry=2)
at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/lj_api.c:1151
#8 0x000055555555c9a6 in docall (L=L@entry=0x7ffff7cb6380, narg=narg@entry=1, clear=clear@entry=0) at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/luajit.c:122
#9 0x000055555555d221 in handle_script (L=L@entry=0x7ffff7cb6380, argx=argx@entry=0x7fffffffd6a0) at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/luajit.c:292
#10 0x000055555555d904 in pmain (L=0x7ffff7cb6380) at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/luajit.c:550
#11 0x00005555555b8396 in lj_BC_FUNCC () at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/buildvm_x86.dasc:857
#12 0x00005555555af1c0 in lua_cpcall (L=L@entry=0x7ffff7cb6380, func=func@entry=0x55555555d7af <pmain>, ud=ud@entry=0x0)
at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/lj_api.c:1179
#13 0x000055555555d9c4 in main (argc=3, argv=0x7fffffffd698) at build/x86_64-linux-gnu-debug/thirdparty/luajit/source/src/luajit.c:581
(gdb) p c
$2 = 54
(gdb) p name
$3 = (const lChar32 *) 0x5555571b3e60 U"FictionBook"
(gdb) p m_by_name[54]
Cannot access memory at address 0xe0409000101b3
(gdb) p m_by_name[1]
Cannot access memory at address 0xe04090001000b
No idea yet how/why this.
There was a problem hiding this comment.
So tell us
You are so busy at the moment, didn't want to bother you with the C code.
I'll add the comment.
| }) | ||
| table.insert(style_table, { | ||
| text_func = function() | ||
| return _("Preformatted text in TXT files") .. (G_reader_settings:has("txt_preformatted") and "" or " ★") |
There was a problem hiding this comment.
Just thinking aloud a bit.
Preformatted text means spaces and such are kept rather than collapsed, imo this is what people expect as "normal" TXT behavior. Preformatted as a term primarily makes sense in the context of HTML, LaTeX or some such where you tell the program to stop doing things like collapsing spaces but just to render it as is instead. Additionally it's a bit of technical jargon, maybe a simple Render text as is would do the trick.
Given the above would it make more sense to revert it, for example as Typeset TXT files? Although that's not quite right.
Other potentially relevant terms include fixed width.
That also raises the question as to whether the "special" behavior's actually desired or if the users in question just want a regular non-fixed-width font.
There was a problem hiding this comment.
Agree that "preformatted text" is not very clear.
I remember the discussion here that the main purpose of txt files (in the Western area) is the ASCII art.
There was a problem hiding this comment.
ASCII art is a bit too specific really, even just some indentation or a basic table is mangled by the special behavior iirc.
But yes, here's a common type of text file:
DOSnDonts-017.txt
There was a problem hiding this comment.
It can be "Apply style to TXT files" with default unchecked.
There was a problem hiding this comment.
It's not really about style though. :-) Using a regular non-fixed-width font doesn't mess with it the way the crengine special behavior does.
|
A few random notes having a look at "txt preformatted" handling in crengine: The alternative to "preformatted" is actually not just non-preformatted, it's auto-magically-detect-txt-format-layout (that I mentionned elsewhere): /// text format import options
typedef enum {
txt_format_pre, // no formatting, leave lines as is
txt_format_auto // autodetect format
} txt_format_t;There's a lot of history and magic (and possibly false decisions on some books) in that bit of code, that I promise I will never look at :) Using one or the other of these formats, it is the only thing that I know of in crengine that makes and keeps 2 distinct cache files: But even if this has (or should, haven't looked at it yet) effect only on txt files, the flag is used also with other formats to end up in the cache filename, even with EPUBs: so I end up with (same size, different content): and these caches files will be rotting around (I remember we store one in the metatada.epub.lua and that we clean it on occasion). So, I don't really know if we should introduce the above issues with enabling toggling this crengine feature.
|
|
Default state for TXT:
|
poire-z
left a comment
There was a problem hiding this comment.
Some suggested comments to keep track of what I understand (or failed to understand :/).
| end | ||
| end | ||
|
|
||
| function ReaderRolling:showReloadConfirmBox(warn_txt) |
There was a problem hiding this comment.
May be showSuggestReloadConfirmBox() ? as it has some hardcoded message in that sense, it's not a generic reload document thingie.
| or G_reader_settings:readSetting("txt_preformatted") | ||
| or 1 | ||
| if self.ui.document.is_txt then | ||
| -- default to disable TXT formatting as it does more harm than good |
There was a problem hiding this comment.
-- Default to no fancy detection and formatting, leave lines as is
| or G_reader_settings:readSetting("txt_preformatted") | ||
| or 1 | ||
| else | ||
| -- for other formats, it affects crengine cache |
There was a problem hiding this comment.
-- For other formats than txt, we should keep this setting fixed or it could create multiple cache files
| -- setting txt_preformatted for the opened document causes segfault, hence reload | ||
| self.ui.rolling:showReloadConfirmBox() |
There was a problem hiding this comment.
-- Calling document:setTxtPreFormatted() here could cause a segfault (there is something
-- really fishy about its handling, like bits of partial rerenderings happening while it is
-- disabled...). It's safer to just not notify crengine, and propose the user to reload the
-- document and restart from a sane state.
There's something really fishy I can't figure out... and won't have time, so fine with reloading the document and pretend there's no fish :)
| sub_item_table = obsoleted_table, | ||
| separator = true, | ||
| }) | ||
| if self.ui.document.is_txt then |
There was a problem hiding this comment.
Thank you for that - and not showing this hard to explain item to people who don't read TXT files.
Might be worth having a help_text though. Quick proposal (possibly not accurate):
When enabled, the rendering engine will use heuristics (text wrapped under some max line length, single or double blank lines marking paragraphs, indentation or centered text considered heading...) to guess how to better render the text.
When disabled, the text is rendered as is in a monospace font, which may be best if it has some ASCII art drawing or format things with spaces.
|
Added all comments, except help text because long-pressing is reserved to make it default without additional ConfirmBox. Asking @offset-torque to add a short description to the "TOP MENU → Style" chapter of the guide: Auto-detect TXT files layout This menu item is visible when a txt file is opened only. |
|
Don't we have somewhere some long-press ConfirmBox with some additional info in its text - where we could put this text ? |
|
Not yet, it requires MultiConfirmBox. |
|
I am adding, thanks for already writing the text. So the aim is to make txt files more readable by formatting it a bit? I guess most of the time users would want this enabled, unless there is formatting they want to keep as you said. |
|
Imo it absolutely destroys any and all text files I've ever tried and you never want it enabled. |
Soo let's complete the sentence:
There should be a benefit, right? Otherwise why this effort. |
|
There are aspects of it that are okay, a bit like Markdown. It just does… more. And perhaps somehow not enough at the same time. :-) Though mind, there's the "obsolete" txt.css that may or may not look a tiny bit better in some cases.
What I think people want is a regular non-fixed-width font option, without completely collapsing spaces and such. (And some of the stylistic replacements can be overly enthusiastic, though I can't find an example right now.) It would break ASCII art, but it wouldn't break the TXT file. 
|
|
Thanks, @Frenzie! Cool illustration. So basically this PR is to support quite old format of books: there are in txt without any formatting involved. If I remember it correctly, this was done to have very low requirements for the reading apps back in the days of PDAs - personal digital assistants in early 2000s. I remember reading a book or two on some HTC mobile phones :D Those are polar opposite of text files designed for monotype console fonts - where all those fancy graphics spelled out by symbols are possible. My 2 cents is that KOReader is mainly a reader - I presume books as in literature, not coding text files and such :D I only read one book in txt and I remember making a style tweak to make it bearable to read. I will share it here later |
|
The terrible effect on books is why I disabled this in the first place, but for the record the above newsletter full of articles meant for reading was published mere days ago.
|
|
Incidentally, if you try to select the title that was too embarrassed to show up you get a crash. Edit: it's probably self-evident, but here's the document: https://www.gutenberg.org/cache/epub/45839/pg45839.txt |
|
To be abundantly clear, I've marked all the spots that are completely missing (x), completely broken (o) or incorrectly positioned (_). |
|
Can reproduce in one place of the book only Strange xpointer |
With txt.css it looks differently but broken as well. |
I guess it's allowed to xpoint to an empty element.
txt.css is old and crap (a clone of fb2.css with may be or not old stuff not removed or new fb2.css stuff not added). But maybe you're expecting too much from this old code from 15 years ago that we haven't visisted since, which maybe lag and will fail will all the work we've done since :/ I'd really like to see this autoformat stuff do some magic with some specific txt file where it would detect things with success and really do better than preformatted. A style tweak with |
I wrote much the same thing back in 2015. #1607 (comment) In the end I kept it with the fixed width font by default for better compatibility. |
|
I think we can safeguard |
|
A choice between working properly and completely broken is not worth having. What specifically is it that these Chinese and Cyrillic users are hoping to get out of this? |
|
fb2.css indeed performs better, because the other one would eat |
Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>
|
|
||
| local old_text = self.selected_text and self.selected_text.text | ||
| self.selected_text = self.ui.document:getTextFromPositions(self.hold_pos, self.holdpan_pos) | ||
| if self.selected_text and self.selected_text.sboxes and #self.selected_text.sboxes == 0 then |
There was a problem hiding this comment.
A little comment about when this can happen ?
| end | ||
| end | ||
| provider.is_fb2 = file_type:sub(1, 2) == "fb" | ||
| provider.is_txt = file_type == "txt" or nil |
There was a problem hiding this comment.
Is this or nil really needed ? (and why is_fb2 is fine with false and is_txt not ?)
There was a problem hiding this comment.
To save a couple of bytes of memory in 99.9% of cases. May be more, says ChatGPT.
In Lua, a boolean variable (true or false) typically takes 1 byte of memory for storage. However, due to Lua's internal memory management and the way Lua handles values dynamically, the actual memory consumption may vary depending on the Lua implementation and the underlying system architecture.
Lua represents all values as tagged variants (also known as tagged unions), meaning a boolean is stored within a structure that includes type information. If you are using a standard Lua implementation (such as PUC-Rio Lua), the internal representation of a boolean might take more than 1 byte due to memory alignment and Lua’s internal data structures.
There was a problem hiding this comment.
Lua as such isn't terribly relevant. But without checking I wouldn't know what LuaJIT does here specifically. :-)
There was a problem hiding this comment.
It's one per document, and so very little compared to everything else :)
(Also, I'm not certain looking up the key is_txt in the provider table is cheaper when it doesn't exist - need to iterate all keys to see it's not there - than just finding it and stopping the iteration).
But I'm more interested in readability and consistency than any kind of microoptimisations (and given our large code base and the kind of software this is (a Reader where we are idle most of the time), I think readability and consistency matter most.
Consistency being that if you add it for is_txt, it should be there for is_fb2 in the line just above - so any reader of this code should not wonder why there are different, and if there are differents there may be a reason, and wtf is the reason ?! - like I did :)
|
Works great!
With this PR I just enabled it once and it all worked! |
|
I see this is kind of a hot topic now so benefits are debatable I guess. But considering mergen3107's observation above, probably I can say something like this in the guide (open to corrections):
|
|
I think you accidentally didn't squash. No biggie, but letting you know in case you didn't notice 'cause it's a bit messy. |
|
Thanks, the default button has changed in the web interface, it was "Squash and merge" previously. |
|
@hius07, thanks a lot for this change. Now I can read TXT files in KOReader as well instead of the built-in Pocketbook reader. |
|
Good to know, thanks for the feedback. |
|
@hius07 : do you remember why the need for this: ? koreader/frontend/apps/reader/modules/readerhighlight.lua Lines 1898 to 1904 in 3e6d50f Been looking at #12135 again, and I often when testing (on some EPUB, haven't tried a TXT) and possibly moving back my cursor/finger to where it was when I started the selection, I so get an empty selection while I'm still panning - so I'll possibly be continuing my selection to get something non-empty when I release it. I feel that in this onHoldPan, if we get empty sboxes, we should just ignore/do nothing (highlight nothing): on the next onHoldPan we may get something. It's in onHoldRelease that we may want to abord if the sboxes are empty/there is no content selected to highglight or work with. |
Empty selection, when selected_text is not nil, means #sboxes is 1 (not 0). It's a zero sbox (x,y,h,w all 0). So the condition is not met and the process is not aborted. |
|
Yes, I can - at least with the changes I made - which may have issues, that I may fix - but I think this should not get in my way :). 
I get: I think it might be when the selected text (between the 2 xpointers I'm trying to make better) is a space (maybe it's something that never happened previously, but it feels that (even it I later feel it might be a bug and that I should fix it) it might be a legitimate outcome that frontend should not just give up and abort the current selection, but only deal with it - possibly ignoring the selection - when onHoldRelease. Why did you feel the need to add that in onHoldPan ? What kind of issue did you have before adding it ? |
|
I don't remember exactly, sorry. Some txt files caused problems, maybe my solution was not perfect. |
Is this a new behavior? |
|
But even when highlighting a single space, shouldn't sboxes be not empty? |
Possibly, dunno. It's happening with my current work in progress, it's possible that when I'm done they will keep happening, or that they won't - can't say yet :)
I dunno. Logically, we can have a pair of xpointers pointing to a single (or multiple) space. May be frontend or base/cre.cpp with it's extension to word start/end avoid this. (
I dunno, it might be that getSegmentedRects tries to ignore them (so boxes adjust to the words, so we don't get blank on the sides, which would be ugly). May be this has some strange effect (like the above) when the selection happens to only be a space. |
I removed the selection empty check from onHoldPan in the 2nd commit of #14979 . |
|
Okay |
Expose txt_preformatted to UI
To help chinese and cyrillic users who read a lot of txt files.
This change is