search

Malanche / escpos-rs

Control esc/pos printers through rust.
MIT License
15 stars 5 forks source link

Make Printer::table_2 more generic #2

Closed DusterTheFirst closed 2 years ago

DusterTheFirst commented 2 years ago

I want to start by thanking you for all your work on this project so far. Coincidentally, I have just gotten into hacking an EPSON thermal printer and this library has been a huge help for me. I seem to have gotten into this at just the right time to use this library. Since I am using the library I thought it would be good to send some small ergonomic fixes up to you to hopefully help anyone else who wishes to use this amazingly featured library. Anyways, with the fluff out of the way, here is the actual pull request content.

This change makes table_2 be able to accept more than just a Vec<(String, String)> which is what it was constrained to before.

The function can now take in anything that implements IntoIterator including iterators themselves and, as before, Vec. The function is also now generic over the type of the elements in the tuple, allowing for anything that implements AsRef<str> (String, &str, Cow<str>, ...) this functionality is the same of that in the Printer::println function and makes writing code much easier since callers do not need to deal with tons of .to_string()s on their string literals

DusterTheFirst commented 2 years ago

As a side note, I would recommend running rust's formatter cargo fmt (and the linter cargo clippy) on your codebase. I use VSCode as my editor with rust-analyzer which auto formats my code for me hence the changes all over the place in this PR. I had not realized that I had run rustfmt. The important changes are these: https://github.com/Malanche/escpos-rs/pull/2/files#diff-45d1638e21d07e9839acd6a40836cca85f5ea8f7368cda33cc3b0e0cceeb27a1L175-L190

https://github.com/Malanche/escpos-rs/blob/9ad251321f5664f49ea0205014d1fba63ecbc354/src/printer/usb_printer.rs#L186-L205

DusterTheFirst commented 2 years ago

Side side note. currently, the table_2 function assumes a width of 30 chars, but that is not the case with my printer, so it might be useful to make that calculated wither using the PrinterProfile.width and the font size or something. Or just letting users enter it every time this function is called. Also, the table_2 function has no way to handle if the sum of the 2 columns is > the total width. Right now it will panic with attempt to subtract with overflow. the ESC/POS system might have a way to right align text in the printer that could help with making this less of a problem.

Edit: After looking at the library more, this function seems to be the same as the duo table instruction. Those functions should probably also be made as generic as this one. If this PR gets merged, I would be more than happy to work on those others as well.

Malanche commented 2 years ago

Hey, hello! Thanks a lot for the help. I am happy the library seems to be of help to somebody.

I will take into account your changes. I rushed to upload the crate as the name was free, but there are many things to be done that were hardcoded before (I used the library to help my family so it was not general purpose, it was really specific for certain printer).

The printing width now comes as part of the PrinterProfile structure (I only have two models to test, a 58mm paper and a 80mm one. You can see both models in PrinterModel).

As an example, the Instruction structure takes into account printer width for both pictures and text depending on the font... there is still a lot to be done but I will quickly put it up to date. I will also update the Printer functions to take into account the profile. I might use python's escpos database to have mor predefined profiles. Thank you again!

Malanche commented 2 years ago

The changes to the table are on their way, by the way (I have local changes to not have redundancy anymore in the code, so the table printing and so is abstracted from the to_vec function, so that it can get used by the printer). Functions like duo_table already have generic parameters on them in my local, but I'm still using a vector of tuples for the contents of the tables (didn't know about the IntoIterator trait). As soon as I have them, I come back to the pull request to merge your suggestion.