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

Desktop: Support export of multiple notes to pdf files. #2468

Merged
merged 7 commits into from
Feb 11, 2020

Conversation

miciasto
Copy link
Contributor

@miciasto miciasto commented Feb 8, 2020

As requested and discussed in forum here

Comments

The change affects export to pdf launched from all locations:

  • main menu
  • right-click context menu in note list
  • context menu in note-text pane

If one note is selected:

  • the user sets the filename (as before)

If multiple notes are selected:

  • the user sets the destination folder
  • the filenames are based on the note titles and folder title eg 'My Note - My Notebook.pdf'
  • if the filename already exists, the filename will have a number inserted
    eg My Note Title (2).pdf

Some of the modified code is also used by the print function, however print behaviour should be unchanged.

Testing

Tested manually on desktop (Linux)

Check export of multiple notes to pdf:

  • using note-list menu (right-click)
  • using note-text menu
  • using main menu
  • where notes are in same notebook and have same title
  • where output file already exists

Check export of single note to pdf:

  • using note-list menu (check it still works)
  • using main menu (check it still works)
  • where file exists (check it still gives user warning)

Check print:

  • single note (check it still works)
  • multiple notes (check it is still disallowed)

Edit: Updated after changes in response to review

@miciasto miciasto marked this pull request as ready for review February 8, 2020 06:30
@laurent22
Copy link
Owner

Looks very good, thanks a lot @mic704b. I've just noticed two small issues and then we can merge.

@tessus tessus added the desktop All desktop platforms label Feb 8, 2020
Find unique filenames when exporting multiple pdfs.
Simplify delay.
@miciasto
Copy link
Contributor Author

miciasto commented Feb 8, 2020

Thanks for the review @laurent22. I've updated in response, with a question above.

I also made a small mod to the fs shim -- please check it carefully.

The loop appeared to try the same last-resort filename unnecessarily many times. There was no comment justifying why, so I assume it is a mistake and fixed it. The only reason I can think it was like that, is that some filesystem is unreliable and will provide a different response if you ask it enough?

@laurent22
Copy link
Owner

The loop appeared to try the same last-resort filename unnecessarily many times. There was no comment justifying why, so I assume it is a mistake and fixed it. The only reason I can think it was like that, is that some filesystem is unreliable and will provide a different response if you ask it enough?

I think it was me being a bit too careful. If that function is called multiple times from several places within the same milliseconds I guess two files could end up having the same name. Maybe let it loop a few more times just to be sure? (I guess that code will almost never be executed anyway, so it's not like it will slow anything down)

Merge branch 'master' into feature/export-pdf-multi
@miciasto
Copy link
Contributor Author

Maybe let it loop a few more times just to be sure? (I guess that code will almost never be executed anyway, so it's not like it will slow anything down)

You're right, it is code that may never get run!

Anyway, it's there, so I did a test run on an instrumented version, and on my system the loop runs at least 10 times every millisecond. So with 10000 loops it was effectively only trying max 1000 different filenames.

I hope you don't mind, I put it back to 100 extra loops, but inserted a 10ms sleep so the filename tried is changed in each loop. So it now tries 100 extra times and will take about 1 extra second before it gives up.

@laurent22
Copy link
Owner

Looks good, thanks @mic704b!

@laurent22 laurent22 merged commit 573b744 into laurent22:master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants