Set major-mode for org-journal pop-up buffer to org-mode

[drfiresign]

Credit to @zhyzky for pinpointing the issue and describing the fix in #414. There's a slightly different macro described in the issue report. I wasn't able to get it working however; I opted instead to just take the solution and modify the existing code as minimally as possible.

I'm happy to let @zhyzky optimize this macro for better performance or simplicity, I just wasn't able to get their solution working in the few minutes I was tinkering with it.

Cheers

[drfiresign]

Thanks @zhyzky, I appreciate the second set of eyes 👍🏻

[bastibe]

I'm afraid this change might have huge performance impact on searches. We use this function as part of the org-journal-search facilities, and we explicitly search through files in fundamental-mode, as org-mode would make searches extremely slow. For reference, searches often involve hundreds or thousands of files, and org-mode can take up to a second to initialize. That's just not acceptable.

Could you check whether search performance is impacted by this change?

[zhyzky]

I'm afraid this change might have huge performance impact on searches. We use this function as part of the org-journal-search facilities, and we explicitly search through files in fundamental-mode, as org-mode would make searches extremely slow. For reference, searches often involve hundreds or thousands of files, and org-mode can take up to a second to initialize. That's just not acceptable.

Could you check whether search performance is impacted by this change?

I understand your concern，and would like to know how to test the search speed. However, this pull request here would redefine the major-mode as a local variable, that is, it won't affect the font-looking or anything else. So it won't slow down the performance, not too much at least I believe.

[drfiresign]

I'm afraid this change might have huge performance impact on searches. We use this function as part of the org-journal-search facilities, and we explicitly search through files in fundamental-mode, as org-mode would make searches extremely slow. For reference, searches often involve hundreds or thousands of files, and org-mode can take up to a second to initialize. That's just not acceptable.

Ah, that is understandable. I definitely think we could use some pointers on how to test this, but I also wonder if you have a better idea on how to deal with this particular change in org-mode mechanics. If this isn't the right way to approach handling this error, I'd certainly appreciate some advice about how best to avoid this particular disruption.

[bastibe]

To test this, create a journal directory with a few hundred daily files with some random content. The files may contain lorem ipsum, or they may each contain the exact same content, it doesn't matter much. But they should contain some text. Then run an org-journal search on them. And remember that any slowdown encountered on a modern SSD-equipped computer will be infinitely worse on old HDD machines.

I may be wrong. Perhaps org-mode has gotten a lot faster, or uses better caching, and this is no longer an issue. But I remember we coded the org-journal-search specifically in this way, since the regular org-agenda searches were unbearably slow at the time.

As for advice, if I'm reading this correctly, there's an issue with calling org-element-at-point. Essentially, we shouldn't use any org functionality as part of the search. If we do, that's a bug. The whole point of the search (originally) was that it doesn't involve org, and is therefore fast.

If org functionality is required for some uses of org-journal--with-journal, but not searching, then we should create separate functions for these two use cases.

[zhyzky]

To test this, create a journal directory with a few hundred daily files with some random content. The files may contain lorem ipsum, or they may each contain the exact same content, it doesn't matter much. But they should contain some text. Then run an org-journal search on them. And remember that any slowdown encountered on a modern SSD-equipped computer will be infinitely worse on old HDD machines.

I've got like 34 weeks journal file, and test result like this (benchmark-run 10 (org-journal-search-forever "somestring")) (0.748114 1 0.09099299999999999) --> original (0.769555 1 0.09732099999999999) --> with hack (0.731607 1 0.08872999999999998) --> with additional hack the third result is due to changes like this

       (setq result (let ((major-mode 'org-mode)
                          (change-major-mode-hook nil))
                      (progn ,@body))))

So the performance reduce by this hack is accepable, right?

I may be wrong. Perhaps org-mode has gotten a lot faster, or uses better caching, and this is no longer an issue. But I remember we coded the org-journal-search specifically in this way, since the regular org-agenda searches were unbearably slow at the time.

As for advice, if I'm reading this correctly, there's an issue with calling org-element-at-point. Essentially, we shouldn't use any org functionality as part of the search. If we do, that's a bug. The whole point of the search (originally) was that it doesn't involve org, and is therefore fast.

If org functionality is required for some uses of org-journal--with-journal, but not searching, then we should create separate functions for these two use cases.

Now I get your point why to design a journal mode, that is to use the org-mode to edit (a) journal file while to search (several) files under fundamental mode. That would be a more complex project.

[bastibe]

Fascinating! Utterly fascinating!

I remember how searching used to take minutes with org-mode enabled, and less than a second with it disabled. I ran the benchmark on my machine as well, on my 1200-file journal, and I can confirm that the performance impact is negligible.

Could it be that (setq major-mode 'org-mode) is not actually activating org-mode, but is still fixing the warning somehow?

At any rate, I have no further reservations about this change. Sorry for the confusion.

[drfiresign]

Great! @zhyzky you did all the work here I just tried to make the PR, did you want to open a separate PR for this or use the Suggestion feature to add change-major-mode-hook?

[zhyzky]

Could it be that (setq major-mode 'org-mode) is not actually activating org-mode, but is still fixing the warning somehow?

Yes, change a variable value not activating org-mode, which may be different as before. The main difference here is to use a (let) structure, where a temperary (local) value of 'major-mode' is defined inside the block. Not the (setq), which would change the major-mode (globally). I think some function would not be called when using a local binding. And the 'change-major-mode-hook' the same. I was just guessing that with (let) form, the 'change-major-mode-hook' may be called, so I give it a try. The result here doesn't have some obvious effect. So @bastibe, if you're pleased with this patch, could you please just modified the code when you approve the pull request?

[bastibe]

I would ask you to add a comment that describes the reason for the let at this point in the code. After that, I'll happily merge it.

[nuthub]

I'd love to see this patch merged into the main branch. For me it just works. Is the pull request stuck because of the missing comment?

[bastibe]

Yes, actually. Feel free to open a new pull request and add the missing comment, and I'll merge that one.

[drfiresign]

Hi folks, sorry for the delay here; I've updated the change with a brief comment. Let me know it this is unacceptable for any reason.

[bastibe]

No worries! Looks good from my end. Is this ready to merge?

[drfiresign]

@bastibe Yes, this is ready as far as I'm concerned 👍🏻 Thanks for your help!

[nuthub]

thank you!