Details
- Reviewers
svuorela - Group Reviewers
Frameworks - Commits
- R280:6fb9f3f5b21f: Add QML support for Prison
Diff Detail
- Repository
- R280 Prison
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Cool in principle, few nitpicks.
| src/quick/barcodequickitem.cpp | ||
|---|---|---|
| 38 | Is createBarcode a heavy function? It sounds like it could be. If so I'd recommend using QQmlParserStatus so you don't generate it 4 times on startup as each property gets set. | |
| 114 | you have the actual up-to-date item size from inside paint. Would that be better than minimumSize? Please also change to img = m_barcode->toImage(size * qApp->devicePixelRatio()); for high DPI support. | |
| 132 | You're doing this in the paint, do you need to do this here? | |
| src/quick/barcodequickitem.cpp | ||
|---|---|---|
| 38 | Right, that's cleaner, I'll fix that. | |
| 114 | I don't think any of this makes a visual difference here, as the image is ultimately just a black/white pattern that is scaled with nearest neighbor scaling anyway, we'd just be moving that scaling around a bit. Results look sharp here at 1x/1.5x/2x scaling and are detected correctly by the barcode scanner. | |
| 132 | That's an issue with Prison, you only get a valid minimum size after one call to toImage(). I'll add a comment about that. | |
Cool! Would allow us to get rid of the custom barcode stuff in Plasma's clipboard plasmoid.
Does barcode generation need to be in a separate thread? The Plasma codes does it, not sure how big of a performance impact this has?
| src/quick/barcodequickitem.cpp | ||
|---|---|---|
| 145 | Use setImplicitSize to do both in one go and avoid intermediate signal emissions | |
| src/quick/barcodequickitem.h | ||
| 91 | QQuickPaintedItem inherits QQuickItem which has a isComponentComplete() method, no need to keep track of that yourself | |
I haven't profiled this, but re-generating the barcode at the key repeat rate with the QML test included in this commit shows no measurable CPU cost or rendering lag on a desktop machine for any of the code types.
I am a bit unsure if this is the right approach. I can still be convinced both ways.
one of the big differences from prison/qt4 to prison/qt5 was that it changed from being a barcode display library to a barcode generation library, removing all means of actually displaying them.
I don't know if a better approach would be to still not actually display anything, but do a bit more to make it possible to just use a AbstractBarcode in a QImageItem.
I am also not sure how a custom AbstractBarcode could be fitted in here, and if it is actually relevant to care for a thirdparty AbstractBarcode implementation.
This is a bit of rambling, but I'm not fully sure what way to sway. The code as such is good, but I'd like to just bounce these questions around first.
The interface for outputting barcodes is currently QImage. The problem is that QtQuick cannot directly consume a QImage without custom glue code (image provider, custom item, etc), unlike e.g. a QLabel. Using an image provider would allow standard Image elements to consume the barcode, I decided against that approach though as the "API" in that case is an URL, ie. you'd need to encode the content, the type and any other parameter inside a single URL. This might be conceptually cleaner, but as an "API" it is just plain ugly.
Supporting custom AbstractBarcode subclasses is indeed something I haven't thought about. It would probably require you to obtain an instance somehow and pass that into the Barcode element as an opaque handle as far as QML is concerned. This would need an additional property on the Barcode element, not a big deal I think. I'm reluctant to add this blindly now though, given I have no actual test-case for this.
Therefore, for me the choice here is mostly about where to put the necessary glue code, with the alternative being my application (I don't think a tier2 KF5PrisonQuick makes sense for this). With Plasma apparently having similar needs sharing this somewhere seems sensible though.