Skip to content

Conversation

@JohannesLorenz
Copy link
Contributor

Should I merge this to master? It's practical and may help coders in the future, but it's also currently unused code (as it's only inside my osc-plugin branch), so coders have no example about how it must be used.

@JohannesLorenz
Copy link
Contributor Author

Note: CI only failed because of a used assert() without an include. I'll fix this soon (it looks like it's Q_ASSERT is common style?).

@PhysSong
Copy link
Member

Q_ASSERT crashes the program if not built with QT_NO_DEBUG. Currently we don't define it in release builds so Q_ASSERT will always crash LMMS.

@LmmsBot
Copy link

LmmsBot commented Sep 22, 2018

Downloads for this pull request

Generated by the LMMS pull requests bot.

@JohannesLorenz
Copy link
Contributor Author

@PhysSong Thanks for the hint. Aborting if a pixmap is not loaded sounds like overkill, even in debug mode. I changed it into a warning now in e0384b8 , and the code will safely run on ...

Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I've suggested some improvements, but I wouldn't bother if you merge this as is.

QImageReader reader(QString("artwork:%1").arg(pixmapName));

// xpm strings are not loaded from file, you must use addXpmToCache()
if (pixmapName.startsWith("xpm:"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better if this check were before creating a QImageReader instance.

include/embed.h Outdated
* @param pixmapName Unique identifier for the XPM. Do *not* prepend "xpm:".
* @param xpm_data The raw XPM string (*not* a file name).
*/
inline void addXpmToCache(const QString& pixmapName, const char** xpm_data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can only be called while a GuiApplication is running.

Can we check it in this function and warn if the condition not met?

@JohannesLorenz JohannesLorenz merged commit 4babbe2 into LMMS:master Oct 11, 2018
@JohannesLorenz JohannesLorenz deleted the xpm-pixmaps branch October 11, 2018 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants