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

[Requst for Help] Please test dev/nov branch for backward compatibility before merge #66

Closed
nobiot opened this issue Jul 15, 2023 · 24 comments

Comments

@nobiot
Copy link
Owner

nobiot commented Jul 15, 2023

I am about to merge a development branch dev/nov that has been in the works for 6-7 months. It will add support for EPUB books with nov.el and open up more possibility to support non-file-visiting major mode such as Info.

The dev branch contains a lot of refactoring. I have been trying my best to make this change as smooth as possible for existing Org-remark users. I believe there is no break changes in the eye of users. My tests have been good so far. My old notes file work with no adjustments.

It would be great if anyone could spare some of your time to test the branch to see there is any backward compatibility issues. I intend to merge it with the main branch once two or three volunteers have their eyes on it and issues resolved.

What would be great is if you simply open the files (or webpages) where you have highlights, do things you would normally do, and see if anything breaks.

As the new code should be backward compatible as is, you should not have to do any extra config. And your notes should load just fine.

Nevertheless, there will be a few visible changes with additional features (please see below).

I am excited about the new features and will really appreciate if you could help me bring them to whoever benefits from them as smoothly as we can.

Thank you.
nobiot

1. "Icons" for highlights with annotations

Screenshot from 2023-07-15 14-09-52

The "icon" is a string at the moment and you can customize it with org-remark-icon-notes. If you set it to nil, you can disable it.

2. Automatic adjustment of highlight positions when different from original

Screenshot from 2023-07-15 14-20-08

This is a new feature that is originally designed to tackle specific rendering issue of nov.el (issue #50). Now it has become a generic feature. I think it would work nicely for webpages when they are edited as you can see in the example of Wikipedia in the image above.

I have introduced another icon to indicate that Org-remark has automatically adjusted the position. You can customize the "icon" string with org-remark-icon-position-adjusted (setting it to nil disable the icon but the adjustment still occurs) and face with org-remark-highlighter-warning.

If you are interested, more technical detail is described in this section of the user manual (already publicly visible).

3. How to set up the new EPUB support

image

This is not an element for backward compatibility, but you might be still interested in trying it out. It is as easy as adding the following in your configuration (the same mechanism as EWW support as described in README and user manual).

;; Optional if you would like to highlight EPUB books via nov.el
(with-eval-after-load 'nov
  (org-remark-nov-mode +1))

Let me know if you have any questions or comments. Thank you again for reading this.

@mooseyboots @vedang @nanjj @sati-bodhi @randomwangran, @karthink, @holtzermann17, @shombando, @magthe, @linwaytin, @rtrppl, @ryanprior, @ericsfraga, @darcamo, @zhewy, @QMeqGR, @Vidianos-Giannitsis, @AtomicNess123, @ouboub, @dian-yu-luo, @SylvianHemus, @basaran, @Ypot, @oatmealm

@ouboub
Copy link

ouboub commented Jul 15, 2023 via email

@nobiot
Copy link
Owner Author

nobiot commented Jul 15, 2023

Hi Uwe, thank you so much for your quick response.

I just pulled, so you want us to checkout the dev branch, compile and
load and then test it, right?

Yes! It would be great if you could try opening the files that were previously working and see if they still work as if the update had never happened. With your help, I would like to ensure that there is no breaking change to users. So testing the files as you normally use them is very helpful.

Optionally, you could see how the new features work (and I'd appreciate if you could give your feedback), but I would not ask you to spend more time than opening and testing the usual activities with the files you have already used.

Two or three weeks ago I used the package quite a bit (for correcting
matlab exams!)

Is it correct that you were correcting the exams as a teacher for your students? That's a very interesting usage :) Let me know if you have any feedback how Org-remark could help you better with this type of work. Happy to hear your experience if you do not mind sharing.

Thank you again.

@basaran
Copy link

basaran commented Jul 16, 2023

Hi, I don't use remark on a day to day basis but I can test the dev/nov from scratch on my customized doom emacs.

@nobiot
Copy link
Owner Author

nobiot commented Jul 16, 2023

@basaran thank you so much! Let me know how you go with Doom :)

@vedang
Copy link
Contributor

vedang commented Jul 16, 2023 via email

@nobiot
Copy link
Owner Author

nobiot commented Jul 16, 2023

I've been doing both activities on the dev/nov.el branch since yesterday. Everything works as advertised, though I did not experience any of the new features.

Thank you for taking the time doing this and letting me know. I also appreciate your bullet points detail.

@basaran
Copy link

basaran commented Jul 17, 2023

Hello, I had a few remarked files, and they opened fine on the dev/nov.el branch. I also tested epub with nov.el and eww. I am not sure if this is the desired behavior but it seems org-remark is not tracking the individual remarks like it does with org-files.

Here's a screen shot, on a webpage that I have two remarks, but they both open the same marginalia document, and show the same content.

Thank you for making org-remark better and your efforts.

image

@vedang
Copy link
Contributor

vedang commented Jul 17, 2023 via email

@nobiot
Copy link
Owner Author

nobiot commented Jul 17, 2023

Thank you, @basaran.

I also tested epub with nov.el and eww. I am not sure if this is the desired behavior but it seems org-remark is not tracking the individual remarks like it does with org-files

For EWW, this sounds like an expected behaviour. For EPUB, there should be a file in the same directory as the .epub file.

I will come back to this later today when I have more time. Just quickly two things:

  1. The notes file is customized in user option org-remark-notes-file-name. The default is STRING (that is, file name) 'marginalia.org'. The directory is the same as the FILE visited. An option that the user can select in customize is FUNCTION with a default function. For EPUB books, the file should depend on this customizing.

  2. For EWW, the default file name is <user-emacs-directory>/marginalia.org. This is because EWW does not visit a file, thus there is no directory.

I will add a description in README to make it easy to find. I will also review the user manual. The second case might be a bit confusing because I suspect people do not really notice whether a buffer is visiting a file or not.

@Ypot
Copy link

Ypot commented Jul 17, 2023

Highlights for eww don't appear when you visit the website in the future.
If you highlight something new, old highlights appear again.

@ouboub
Copy link

ouboub commented Jul 17, 2023 via email

@nobiot
Copy link
Owner Author

nobiot commented Jul 17, 2023

Highlights for eww don't appear when you visit the website in the future.
If you highlight something new, old highlights appear again.

@Ypot , Is this a problem you didn't have with the current release 1.1.0 and started to have when you tried the development branch?

If not, I suspect it's an issue loading sequence (customizing). I suggest to open a new issue and we can discuss it separately.

nobiot added a commit that referenced this issue Jul 17, 2023
@nobiot
Copy link
Owner Author

nobiot commented Jul 17, 2023

But I obtained the following error messages, and last one worried me.

| In toplevel form: | | org-remark.el:840:18: Warning: ‘org-remark-highlight-save’ is an | obsolete function (as of 1.2.0); use ‘org-remark-highlight-add’ instead. | | In end of data: | | org-remark.el:840:18: Warning: the function | ‘org-remark-highlight-save’ is not known to be defined. emacs --batch | --eval "(check-declare-directory default-directory)"

Thanks for reporting this detail.

This is a weird warning. I get it too. It is caused by this line where I define obsolescence... Because I am making it obsolete, there is no such function.

I will change it to also add an alias, but this still causes the warning.

(define-obsolete-function-alias #'org-remark-highlight-save #'org-remark-highlight-add "1.2.0"
  "Save a single HIGHLIGHT in the marginal notes file. We no longer
save the notes file to disk; hence the name change")`

I have just patched the legacy code warning.

@nobiot
Copy link
Owner Author

nobiot commented Jul 17, 2023

@ouboub, We can disregard the warning -- functionally everything works despite it on my end because there is a new function. I will look if I can somehow silence it. Otherwise, I'd need to remove the line to just remove the function -- not sure if that is a good practice...

@ouboub
Copy link

ouboub commented Jul 17, 2023 via email

@nobiot
Copy link
Owner Author

nobiot commented Jul 17, 2023

BTW, I thought I knew how to set the size of the left margin buffer, or am I mistaken maybe there only exists the possibility to have the org-remark buffer horizontally?

Could you see if org-remark-notes-display-buffer-action might work for you? It might be easier to see how it works from customising. I’ll see if I can improve the user manual in this user option.

@oatmealm
Copy link

I've installed the dev branch and so far I'm not seeing any regression, though I only work with PDFs. Will play around with an eww and epub and will report if there's anything interesting I'm seeign.

Build: grafted, HEAD -> dev/nov.el, origin/dev/nov.el 883f948 2023-07-17 18:48:48 +0200

@nobiot
Copy link
Owner Author

nobiot commented Jul 19, 2023

I've installed the dev branch and so far I'm not seeing any regression

Thank you @oatmealm.

With all the feedback, I feel confident about merging the dev branch. I will update the documentation based on the feedback above and merge -- probably not this weekend but one after...

@nobiot
Copy link
Owner Author

nobiot commented Jul 19, 2023

though I only work with PDFs.

I am considering getting PDF-tool and Org-remark to work together. Still just an idea -- keeping all the capabilities of PDF-tool to interact with a PDF document, save the highlights and annotations in the PDF file as it is now. But have the highlights and annotations sync'ed with the marginalia with Org syntax... This way, we may be able to use the unified Org syntax to write annotations and get the highlights and annotations saved to the PDF document for portability...

Not sure if this is technically feasible. Interested in this direction? If so, I might open a discussion thread once I have something to show -- after the merge, probably few weeks or months down the line...

@Ypot
Copy link

Ypot commented Jul 19, 2023

I am considering getting PDF-tool and Org-remark to work together. Still just an idea -- keeping all the capabilities of PDF-tool to interact with a PDF document, save the highlights and annotations in the PDF file as it is now. But have the highlights and annotations sync'ed with the marginalia with Org syntax... This way, we may be able to use the unified Org syntax to write annotations and get the highlights and annotations saved to the PDF document for portability...

Not sure if this is technically feasible. Interested in this direction? If so, I might open a discussion thread once I have something to show -- after the merge, probably few weeks or months down the line...

I like the idea. It would be like org noter, but in real time.

@nobiot
Copy link
Owner Author

nobiot commented Jul 19, 2023

I like the idea. It would be like org noter, but in real time.

Thank you, @Ypot.
Is your issue resolved?

@ouboub
Copy link

ouboub commented Jul 19, 2023 via email

@nobiot
Copy link
Owner Author

nobiot commented Jul 20, 2023

Sorry but could the document string explain this a bit better?

Thank you for splitting the issue out into #67. My response provided there. Let me if it helps.

@nobiot
Copy link
Owner Author

nobiot commented Jul 22, 2023

The branch has been merged with main. Thank you for your support for this!

@nobiot nobiot closed this as completed Jul 22, 2023
nobiot added a commit that referenced this issue Jul 29, 2023
This issue was closed.
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

No branches or pull requests

6 participants