Skip to content

Notebook file#12699

Merged
hius07 merged 23 commits into
koreader:masterfrom
hius07:notes-file
Nov 21, 2024
Merged

Notebook file#12699
hius07 merged 23 commits into
koreader:masterfrom
hius07:notes-file

Conversation

@hius07
Copy link
Copy Markdown
Member

@hius07 hius07 commented Nov 2, 2024

(After the release)

Closes #12623.

1

2


This change is Reviewable

Copy link
Copy Markdown
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

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

I initially thought: damned, we don't need to answer every 2-users crazy feature request :)
But it seems well done, generic and flexible, adapting to various use cases.
So, thought gone.

Comment on lines +435 to +436
FileManager.instance = self
G_ui = self
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mixed feelings about that new G_ui.

We did without it for years, and introducing it now feels like an adhoc solution (or one being lazy finding another solution :)) and not in line with our object oriented stuff (ie. making the filemanageutil stuff being methods instead of functions - no idea how complicated/ugly this would be vs. this simple and clean solution).
As it also makes other stuff more readable, instead of our multiple ugly local ui = require("apps/filemanager/filemanager").instance.
@NiLuJe : any thought ?

May be it also needs a few G_ui = nil to explicitely make it go away when being done with ReaderUI/Filenamanger, instead of waiting for it to be replaced by the coming up new instance.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Haven't looked at the full picture yet, but, yep, looks nasty and potentially highly error-prone ;).

Copy link
Copy Markdown
Member

@NiLuJe NiLuJe Nov 2, 2024

Choose a reason for hiding this comment

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

Yep, I hate it ;).

Globals are evil to begin with, and this pins the whole object tree in a global, making it doubly so ;).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What is the negative impact, technically?

Copy link
Copy Markdown
Member

@NiLuJe NiLuJe Nov 2, 2024

Choose a reason for hiding this comment

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

Globals are bad form. Globals are also costly.

This also pins the whole tree in memory, because the object in question is the root of basically all of our things, so it holds at least one ref of basically everything.

Add to that that the GC is extremely cautious about collecting globals, IIRC.

That makes for a terrible combination ;).

Comment on lines +190 to +191
table.insert(kv_pairs, { _("Notes file:"), notes_file and notes_file:gsub(".*/", "") or _("Tap to set"),
callback = notes_file_callback })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Each time you add an item to Book information, I need to increase my Info list items per page to have it neat on a single page. Text will become smaller and me blind :/ You're so evil :))

To all: no problem or confusion with naming that feature "notes file" ? We already have Add note / Edit note for associating our own text to a highlight, so this might be confusing.
No other idea about an alternative name for this feature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Notebook file might be slightly more distinct?

Comment on lines +739 to +740
local path_chooser = PathChooser:new{
path = self.ui.file_chooser and self.ui.file_chooser.path or self.ui:getLastDirFile(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What would be the most confortable locations for such notes files ? Alongside books ? In koreader/settings/ ?
Dunno if you could/should store the last directory used for these, so next time one call "Choose", he gets in the directory where all his notes files are (instead of ui.getLastDirFile whatever that gives, or TextEditor last dir, dunno which is picked when).

Also wondering if there should be a default notes_file, a generic one, or the last one used! ? Having to go through "Choose" on each book may be bothering. I'm not a customer for this feature, but if I would, I'd probably use only a single file to append notes about all books I needed to write something about.

@hius07 hius07 changed the title Book: notes file Notebook file Nov 3, 2024
@hius07
Copy link
Copy Markdown
Member Author

hius07 commented Nov 3, 2024

Changes:
(1) Default notebook file (Home/notebook.txt by default, or can be changed and saved to G_reader_setting).
(2) Default notebook file is available in FM via a gesture.
(3) If per-book notebook file is not set, the default notebook file is used.

Per-book notebook file is not set:
3

Per-book notebook file is set:
4

@jonnyl2
Copy link
Copy Markdown
Contributor

jonnyl2 commented Nov 3, 2024

Thanks to @hius07 for your work. I was withholding my review because I was interested to read the original requester @Biep's opinion.

Anyway, here are my thoughts:

  • I would prefer if the default notebook file would be something named specific to the current book. Users preferring a generic book-independent notebook file don't really have a need for this feature anyway; they can already open the last text file (via gesture) or one of the most recent text files (via top menu) as it is.
  • As for the location of the file, it could either be the default export folder, or the respective book's folder (or its sidecar folder, but that doesn't seem popular). I'd prefer the book's folder for the main reason that it will not cause any conflict (e.g., 2 books named 20241103.pdf located in 2 separate folders will cause a conflict if both of them will have a notebook file generated in one common export folder).
  • The filename could just be the book's filename with .txt appended. (And if it's a .txt file opened in the main reader engine, it would end with .txt.txt, should the user decide to add a notebook file.)
  • The thought of not having to go through that relatively time-consuming procedure of having to select a folder and entering a filename for a new text file was one of the main reasons for supporting the original FR.
  • (Personally, I would prefer if the book-specific notebook files would not be globally saved in the list of recently edited text files, as they overwrite the useful list of other recently edited files. But I could certainly see why some would disagree and would like to see all edited files, no matter the context, so no big deal either way.)
  • At least on the AppImage, if I create a new text file from Book Information and then save it, the on-screen keyboard will sometimes stay stuck on the screen after the file is closed, and the only way to make the keyboard disappear is to hit the ESC key.

@NiLuJe
Copy link
Copy Markdown
Member

NiLuJe commented Nov 3, 2024

At least on the AppImage, if I create a new text file from Book Information and then save it, the on-screen keyboard will sometimes stay stuck on the screen after the file is closed, and the only way to make the keyboard disappear is to hit the ESC key.

Possibly hints at yet another VK/InputDialog and/or TE shenanigan. (e.g., either something's not actually torn down properly, or there are multiple concurrent VK instances).

I won't have time to look into it for a few weeks, FWIW.

@hius07
Copy link
Copy Markdown
Member Author

hius07 commented Nov 4, 2024

  • At least on the AppImage, if I create a new text file from Book Information and then save it, the on-screen keyboard will sometimes stay stuck on the screen after the file is closed, and the only way to make the keyboard disappear is to hit the ESC key.

Fixed.

@Biep
Copy link
Copy Markdown

Biep commented Nov 4, 2024

Thanks to @hius07 for your work. I was withholding my review because I was interested to read the original requester @Biep's opinion.

The below seems to agree with my ideas.

  • I would prefer if the default notebook file would be something named specific to the current book. Users preferring a generic book-independent notebook file don't really have a need for this feature anyway; they can already open the last text file (via gesture) or one of the most recent text files (via top menu) as it is.

👍️

  • As for the location of the file, it could either be the default export folder, or the respective book's folder (or its sidecar folder, but that doesn't seem popular). I'd prefer the book's folder for the main reason that it will not cause any conflict (e.g., 2 books named 20241103.pdf located in 2 separate folders will cause a conflict if both of them will have a notebook file generated in one common export folder).

👍️ The probability that both an Alice in Wonderland.epub and a non-notebook Alice in Wonderland.epub.txt exist is vanishingly low.
One generalisation might be to allow .md notebook files, the contents of which show as Markdown as long as they are not actively being edited.

  • The filename could just be the book's filename with .txt appended. (And if it's a .txt file opened in the main reader engine, it would end with .txt.txt, should the user decide to add a notebook file.)

👍️ That would allow the recursive notebook file that I mentioned.

  • The thought of not having to go through that relatively time-consuming procedure of having to select a folder and entering a filename for a new text file was one of the main reasons for supporting the original FR.

👍️ And remembering where which file was, and making typos in the name, and..

  • (Personally, I would prefer if the book-specific notebook files would not be globally saved in the list of recently edited text files, as they overwrite the useful list of other recently edited files. But I could certainly see why some would disagree and would like to see all edited files, no matter the context, so no big deal either way.)

👍️ One option would be submenus for global and local files, so that one would not crowd out the other.

  • At least on the AppImage, if I create a new text file from Book Information and then save it, the on-screen keyboard will sometimes stay stuck on the screen after the file is closed, and the only way to make the keyboard disappear is to hit the ESC key.

No idea what this is about, I am afraid.

The one (low need, low urgency) idea missing here is having the option for several notebook files, characterised by a letter/character, so that one could separate various use cases (or users) that way.
I might want to open notebook file B for my notes, whereas my wife might want to open notebook file E for hers, and the two won't clash. Or I have R notebook files for the reviews, E files for the errata, and so on.

Overall, this sounds great!

@Biep
Copy link
Copy Markdown

Biep commented Nov 4, 2024

I initially thought: damned, we don't need to answer every 2-users crazy feature request :)

True - but this one came from me. 😉

@hius07
Copy link
Copy Markdown
Member Author

hius07 commented Nov 4, 2024

For a new book the notebook file is book-filename.txt, located in the book folder.
Then you can make it default, to be used by other books, by choosing "Use default", or restore using the local (i.e. book's) notebook file by choosing "Use local".

5

@jonnyl2
Copy link
Copy Markdown
Contributor

jonnyl2 commented Nov 4, 2024

Excellent updates. Default local file works exactly as I'd imagined. For users like Biep, who would also like to use alternative notebook files for each book, I just have a few suggestions for making that a bit quicker:

  • When selecting Create, start the file manager from the folder where the book is located (as already the case when selecting Choose)

  • Or, instead of the folder selection process, just start with a dialog prepopulated with the current book's folder, and add a Folder... button for an optional selection process; e.g. like this:
    image
    This way, no folder needs to be selected or changed if the notebook file is just going to be in the same folder as the book, and only the filename needs to be entered. (I'd even go as far as suggesting that the book's filename should also be prefilled, so the user can just add a -A.txt for example, or provide another button to insert the original filename, but I personally probably won't be using any separate files, so @Biep may have any comments)

  • At the moment the user is forced to start editing the file immediately, but at that point he may just want to assign a different file and only start editing it later. I would suggest that only the path/filename is reserved and the text editor not launched at this stage. (So that the actual file is only created when the Notebook file action is launched.)

But as I said, for me it works perfectly fine already, so these are just some completely optional things from my POV.

@hius07
Copy link
Copy Markdown
Member Author

hius07 commented Nov 5, 2024

I'm not a fan of building an interface for several notebook files for a book.

@jonnyl2
Copy link
Copy Markdown
Contributor

jonnyl2 commented Nov 5, 2024

I understand, that is over the top for this very niche use case. The UI is already usable for multiple notebook files at is, as they can be changed easily. My suggestions just tried to address that adding a non-existing file takes many steps and requires typing in an entire, potentially long, filename.

How about this: Change the Create button to Change, and add just one simple dialog box that shows the complete path including filename of the currently set notebook file [edit: or even just the filename, and only allow changing to a filename within the same path the currently assigned notebook file is located in]; e.g. like this:

image

The user can then easily insert a letter or number without having to type out the entire filename or start editing the file. Tapping OK will then just change the property of ["notebook_file"], without creating the file yet.

Only if the user selects Create New... will the current new file creation interface be launched.

@hius07
Copy link
Copy Markdown
Member Author

hius07 commented Nov 6, 2024

On pressing the Notebook file item in the book information window we get the dialog:

11

and then on pressing the Create button we get:

12

@jonnyl2
Copy link
Copy Markdown
Contributor

jonnyl2 commented Nov 6, 2024

Thank you! I think this works well for @Biep's intended use case. Attaching a quick video to demonstrate how one would create several files for the same book:
simplescreenrecorder-2024-11-06_11.43.22.webm

(The currently assigned notebook file can also be edited directly from the reader via a gesture.)

@Biep
Copy link
Copy Markdown

Biep commented Nov 6, 2024

Thanks! That seems to come very close!
I could assign a gesture to the "See associated files" command, and choose the one I want, and then use a gesture for "Open current associated file" from then on, I suppose. (I haven't seen yet how this meshes with the gesture manager, but that scenario seems plausible.)

And I assume that other file endings would be possible - so if one day .md files would be shown as Markdown, I could have a .md annotation file, and see the make-up as long as I was not actively editing it.

@jonnyl2
Copy link
Copy Markdown
Contributor

jonnyl2 commented Nov 6, 2024

No history/record is kept for previously associated notebook files. But they can still easily be switched by going to Book information (possible via gesture), and then selecting one via Choose. Yes, any file extension can be selected or entered.

@Biep
Copy link
Copy Markdown

Biep commented Nov 6, 2024

But they can still easily be switched by going to Book information (possible via gesture), and then selecting one via Choose.

Would there be a gesture for that - for opening the list to choose from? Because that would provide an easy way for what I want/need:

  • One gesture to show the list and allow me to choose the one to associate for now.
  • One gesture to open the currently associated file.

(And in the darkness bind them..)

@Biep
Copy link
Copy Markdown

Biep commented Nov 9, 2024

image

One minimal change (if it is easy to do): place the caret just before the .txt bit, i.e. where any edit actions are most likely to take place. I suppose that with long paths, the caret will always be in view?

Overall, I am very happy with what is brewing here! Thank you, guys!

@jonnyl2
Copy link
Copy Markdown
Contributor

jonnyl2 commented Nov 9, 2024

The file path edit window dynamically expands so the full path and filename will be visible.

The other question is not up to me (I suppose it's easy to do but it may not be the desired behavior for all users).

@Biep
Copy link
Copy Markdown

Biep commented Nov 9, 2024

All right - assuming the full path is not longer than the screen allows. Some people may have instructed calibre or its ilk to save documents to some very information-rich but long path (e.g. in a folder named after the full title within a folder named after all authors). But for me, autoexpand would almost certainly keep the full path visible.

Any ETA on when this would appear in the nightlies?

@hius07 hius07 merged commit 24d8baf into koreader:master Nov 21, 2024
@hius07 hius07 deleted the notes-file branch November 21, 2024 05:53
@hius07 hius07 added this to the 2025.01 milestone Nov 21, 2024
@Biep
Copy link
Copy Markdown

Biep commented Nov 21, 2024

It is there! Wonderful!
I hope to leave for Burkina Faso on 6 December, and stay there a month and a half, with probably no Internet, and with not much in the sense of physical notepaper.
So it is great to have this feature, allowing me to keep neatly-organised notes on many documents, all right there in my e-reader!
I hope to start testing soon; any small quirks might be erasable before I leave.

Thank you, guys! (Are there any women on the team? If so, count yourself included!)

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 21, 2024

The illustrations at the very least.

@Biep
Copy link
Copy Markdown

Biep commented Nov 22, 2024

@Frenzie: Sorry - what do you mean by that? Should I test the stuff shown in the illustrations?

My very first impressions: 🙌👏👍️

  • I created a notebook file (I thought) by clicking the last line in Book information, and choosing Create. This brought me unexpectedly into the editor. (There was also an Edit option, and I had expected that one to create and edit.) I left the editor and connected my e-reader to my computer, to populate the just-created notebook file. However, that file wasn't there. A second attempt, with me adding some text in the editor, fared better.
    The Edit button does the same thing: it asks whether to create and edit, but doesn't actually create unless one makes changes in the editor.
    This is slightly confusing.
    My proposal would be: let either button create an empty file.
  • The idea behind the Create button is that there one can change the filename, isn't it? The name of the button doesn't bring this out, but I am not sure I have a better proposal for a name. Create new, maybe?
  • I assigned the gesture previously assigned to Open last edited text file to Notebook file, and that works like a charm! Great work!
    My proposal: in the menu, move Notebook file near the commands about the text editor, and rename it Open notebook file.
  • Right now the notebook files populate the Text editor menu, which leads to crowding.
    My proposal: have two submenus, Recently opened text files and Recently opened notebook files. That would make two lists, and give more space to each of them.

My overall impression: wonderful job done!

@Biep
Copy link
Copy Markdown

Biep commented Nov 22, 2024

I haven't tried it yet, but I understand I can have several books share a single notebook file too. That is very useful, e.g. the original Hebrew/Greek Bible plus several translations sharing a notebook. In that case allowing Copy/Paste from the file path+name box would be useful - I must check whether that is possible.

@hius07
Copy link
Copy Markdown
Member Author

hius07 commented Nov 22, 2024

(1) Edit opens the assigned notebook file, Create creates another one, as you wanted to have several notebook files for a book. The assigned filename is shown in the input box for easy changes.
(2) One notebook file can be assigned to several books. Just create it in one book, then select the Choose button in another book.

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Nov 22, 2024

Sorry - what do you mean by that? Should I test the stuff shown in the illustrations?

Women. ;-)

@Biep
Copy link
Copy Markdown

Biep commented Nov 24, 2024

Sorry - what do you mean by that? Should I test the stuff shown in the illustrations?

Women. ;-)

I see no women in illustrations, unless you count Alice. But then I may be blind..
More probably I am merely missing the joke.

@Biep
Copy link
Copy Markdown

Biep commented Nov 30, 2024

(1) Edit opens the assigned notebook file, Create creates another one, as you wanted to have several notebook files for a book. The assigned filename is shown in the input box for easy changes. (2) One notebook file can be assigned to several books. Just create it in one book, then select the Choose button in another book.

Thanks. I have little time (preparing my trip to Burkina Faso), but hope to react before leaving.

0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
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.

FR: A command to open "the text file for this document".

6 participants