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

Add Ability For Multi-page PDF -- 1 Keyboard Per Page #64

Merged
merged 4 commits into from
Nov 23, 2021

Conversation

JZL
Copy link
Contributor

@JZL JZL commented Nov 4, 2021

Hi,

I've been enjoying using this to decide which keyboard to build, thanks for building it :-)

The one thing which was missing for me was the ability to print a set of keyboards individually so I could 'try' them out and decide. Sort of like #39. These all get towards that goal (although maybe other small tweaks which might be useful).

The UI tweaks are shown in [1], I added a "Select All Filtered" button (5d81900) to make it easier to select a lot of keyboards at once (which also uncovered an index overflow in the palette selection) and two new "Separate Page" buttons which exports one keyboard per page (25f54bc). I could also imagine keeping only the two export pdf buttons and converting them into a drop down for "Separate" or "All Together"

Finally, I'm happy to help with this but similar to the hotlinking in #54, I thought this led to some possible related features (3e45b08) where a mega pdf is generated with one page per keyboard for people who wanted to compare quickly/print a selection. Also a hot link to a pdf for each keyboard might make it even easier for keyboard creators to link to an easy printing page like the kyria

Per the implenetation:

  • I had to change the way the images are rendered by rmarkdown but it doesn't seem to impact scaling. I printed off a set and they were also 5cm accurate
  • imagemagick does slow down when rendering many keyboards overlayed at once and it also slows down the rmarkdown export for many pages. I don't know how much of a concern it is for shinyapps hosting but this might encourage people to use up a higher server load. Maybe caching? Limit overlay rendering at 10 keyboards?
  • The way I changed it, it also allows printing arbitrary groups of keyboards on a single page (not just all-overlapping or each to a singe page). It might be nice to print two to a page or something, but I couldn't make a nice UI for it

[1]
image

@jhelvy
Copy link
Owner

jhelvy commented Nov 4, 2021

Wow, these are some really nice additions! I really appreciate the time you took to implement them. This is a side project that I have less and less time to maintain, so some fresh edits are welcome.

Some quick reactions:

  • I like the idea of having a hot link for the PDF of each keyboard, but I don't know if putting that in the app itself is the best way to implement it. We could alternatively just write a short R script in the "code" folder that loops through each keyboard and saves the PDF in a "pdf" folder (maybe in "images/pdf", just like I have the "png" folder now). Then the PDFs all live on the repo and have a dedicated url that ends in "keyboard_name.pdf". Basically serves the same purpose, but doesn't eat up the server, plus if the server goes down the PDFs are all still available on the repo, which developers could link to outside of the app. The only thing we have to remember is to re-run that script any time a new board is added, but I usually do that in batches so I would remember to do that.
  • If we did the above suggestion, we could add a little PDF icon in the table on the "keyboards" page that links to the PDF so you could download it from that page as well.
  • Good catch on the color index overflow issue. I guess I had ignored it since I never looked at more than a handful of keyboards at once. More than 3 or 4 and it just becomes a mess. I think your solution solves it because I can't imagine anyone looking at more than 10 or so at once. And yeah, R's 1-based indexing makes the modulo a bit weird, but I appreciate it for data analysis :)
  • I think just "select all" instead of "select all filtered" would be a better name for the button you added, since it does select all of the keyboards, filtered or not.
  • I agree with the choice of limiting the file name...10 seems already too large, maybe just set it to 5? Yes, magic number, but this is a pretty tiny and very specific piece of the app, and it makes sense at a certain point to just name the file "many".
  • I like the "separate pages" buttons. A drop down might be nice though, in which case I would just convert the current label into the main button, i.e. "Print to Scale (PDF)" is the button name, then when clicked the current 4 options drop down to specify the print. I haven't tried making a drop down button, so if it's more work than it's worth we can also just leave it the way it is.

Finally, magick is relatively slow, but I haven't found a better option. Here are some other thoughts I had on it:

  • Implementing some simple caching would probably help and is a low hanging fruit.
  • Another approach I had considered was to simply pre-make all of the overlays ahead of time so that the app is just displaying existing images, which would be very, very fast. Issue there is obviously that the combinatorics get pretty bad pretty fast, even if you limited it to like 10 max boards per overlay. It's still do-able even if you end up with thousands of png overlays, but the repo size would grow considerably to store all those images.
  • Reducing the image size could also help. I use png files because they keep the boards all aligned, so I could maybe reduce the resolution to get an even smaller size. There's a trade off with legibility. I tried using svgs but could never find a way to preserve the alignment.

If you want to play with any of these ideas in this PR, let me know, otherwise I'll go ahead and merge this one and we can consider these other modifications in a different one.

@jhelvy jhelvy merged commit 60cb89c into jhelvy:master Nov 23, 2021
@jhelvy
Copy link
Owner

jhelvy commented Nov 23, 2021

I went ahead and merged this PR as it was in good shape. My comments here I've now put in the todo.txt file.

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.

2 participants