Display error/warning for too large input #72

[ongspxm]

close #72

Currently, for data that is too long, `qrCodeLib.genQr()` will return
an error "Data too long". This exception is not caught, and could be
used to display an error message, warning user that the size of data is
too large for the qr code generation.

The error could be displayed using the existing
`CommonMessages.showError()`, an advantage of tying the error messages
to the `oninput` callback is that when the data size shrinks back to
one that can fits into a qrcode, the area will show the actual qr code
for the data again.

[rugk]

Hi @ongspxm,
thanks for your first contribution to this project! 🎉 👍
I hope you'll like this project and enjoy hacking on it… 😃

So the change itself looks generally good.

However, I am not sure whether it solves #72 completely.
Did you test both the SVG and Canvas version? (Which version does throw this error? Likely not both…)

Also notice you may need to move the code to the specific files for the libraries then. 😄

[ongspxm]

https://github.com/lrsjng/kjua/blob/a88369f748926aee8e996e3f720f0f9534c8d86b/src/lib/qrcode.js#L27

may need to alter the kjua.min file

[ongspxm]

ready for second round of review,

and the kjua thing is kinda hackish, my change is at the end of the min file. search for the only catch in the file

[rugk]

and the kjua thing is kinda hackish

As you noticed, it very obviously is… 😆

And I really don't want to maintain a forked version of kjua or so…
And also, I don't really want to review minified source code. No really…

What I see is this change:

-catch(r){if(!n.test(r.message))throw r}return null}
+catch(r){if(!(a<40&&n.test(r.message)))throw r}return null}

So why only for < 40 chars etc.??
I do not understand that change at all…

So here is what you can do:

• get your change accepted as a PR in the upstream project: https://github.com/lrsjng/kjua/pulls (obviously, you need to explain there why you need this change etc.)
• work around it…?
  Is not there some other functionality/difference that you can see from the outside if the length is too long? (I mean the function you changed should return null in this case?)

In any case, please revert the change to the minified kjua lib…

---

Also in our code base: Please don't throw strings around. When you need to catch them from dependencies (libs) you have obviously no other chance, but here I would recommend custom ES6 error types. They are used multiple times in this project and e.g. the Mastodon addon, so you can get some inspiration from there.

When you use them you can handle the specific "error catching & conversion" in the respective QrLib files and handle the actual error (i.e. showing user error etc.) in UserInterface or so…

---

Also small thing: Do not forget to document when a function throws something.

[ongspxm]

So why only for < 40 chars etc.??
I do not understand that change at all…

Haha, there is 40 versions of qrcode, each accepting more characters than the last. What the lib returns is a canvas or image. And when it runs out of the 40 versions or qrcode, it just returns a blank canvas or image.

There isnt a way of determining if its good, because from the outside, the inage is the same. I could try making a pull request to the library repo.

opened: lrsjng/kjua#12

[rugk]

Okay, then I'll let upstream some time for merging your change. As that, I mark it as blocked and hope upstream will do something.

However, your PR itself still needs some changes as commented before. (e.g. usage of error classes)

[rugk]

BTW, marking this as blocked by lrsjng/kjua#12

[rugk]

Only some minor other changes...

[rugk]

Got unblocked as lrsjng/kjua#12 has been merged 😄

As upstream also updated/changed a little more, can you update the lib to the new version and test whether it works as expected. (Also as for this change here?)

[rugk]

And extra thing: Maybe we can mention somewhere in the source code (or file name?), which (in this case) kjua version we are using?

[rugk]

It's version 0.2.0, not 2.0... 😃

[rugk]

• verified that the included kjua lib is the original one

[rugk]

Oh, I noticed some regression happened:
It does not catch the exception, it just throws with the SVG lib (qrcodegen).

[ongspxm]

@rugk thanks for the fast review, ready again

[rugk]

BTW, considering clicking "resolve conversation" on an issue, you've resolved. 😄 (Then you should also notice, if you forget a review comment. 😉)