Conversation
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
src/TLuaInterpreter.cpp
Outdated
| { | ||
| QString imageLocation; | ||
| if (!lua_isstring(L, 1)) { | ||
| lua_pushfstring(L, "getImageSize: bad argument #1 type (image location as string as string expected, got %s!)", luaL_typename(L, 1)); |
src/TLuaInterpreter.cpp
Outdated
| lua_pushnumber(L, size->height()); | ||
| } else { | ||
| lua_pushnil(L); | ||
| lua_pushfstring(L, "couldn't retrieve image size. Is the location '%s' correct?", imageLocation.toUtf8().constData()); |
There was a problem hiding this comment.
Given that the text is likely to be embedded in a sentence - is it wise to split the text here by inserting a period in the middle? Also, do we want a more nuanced response, that handles all of:
- the file was not found at all
- the file was found but doesn't seem to be an image
- the file was found but it was an image but of null/invalid dimensions
There was a problem hiding this comment.
I don't want to get into all of that - but I've fixed the sentence.
|
|
||
| std::optional<QSize> mudlet::getImageSize(const QString& imageLocation) | ||
| { | ||
| QImage image(imageLocation); |
There was a problem hiding this comment.
Apropos a previous comment - imageLocation is supposed to be a pathFileName so would it not be reasonable to check that it is a valid, readable file first, BEFORE trying to read it as an image?
There was a problem hiding this comment.
Qt will try and fail, and that's what the null check is for later.
There was a problem hiding this comment.
Ah, but won't the image be null both if the file does not exist OR it is an empty (null) image?
There was a problem hiding this comment.
We'll still say we didn't find an image in that very rare case and people will debug from there on
|
@SlySven fixed, have a look again. |
demonnic
left a comment
There was a problem hiding this comment.
tested working with 4 images, fails properly when the image doesn't exist at the path specified
* Add getImageSize() * Tweaks as suggested by peer review on reddit * Review feedback (cherry picked from commit 5b5a203)
* Add getImageSize() * Tweaks as suggested by peer review on reddit * Review feedback
Brief overview of PR changes/additions
Add getImageSize(). Use it like this:
Motivation for adding to Mudlet
So you can size a label appropriately when you download an image from the Internet. This is also necessary for an important script on the pkuxkx game.
Other info (issues closed, discussion etc)
Close #813.
I used a new thing here - std::optional, instead of the usual std::pair<bool, result> pattern - because we have access to it now, and it's much cleaner! See https://www.bfilipek.com/2018/05/using-optional.html about it.