davidedmundson added a comment.

  I love the direction. +++++

INLINE COMMENTS

> ItemCapture.cpp:59
> +        qWarning() << "No previous image found for capture" << d->name;
> +        QVERIFY2(result->saveToFile(dir.absoluteFilePath(filePath)), "Unable 
> to store first version of capture.");
> +        return;

I don't like how we're saving files into the user's source directory directly.

I'm sure we'll get lots of people forgetting to add the files and claiming it 
works because make test works the second time.

And we'll get other people committing lots of _new.png files committed 
accidentally.

----

I think a new temp dir would be best, alternatively we check some env 
variable/arg that a user can set to specify they want to generate a baseline

> ItemCapture.cpp:81
> +
> +    QVERIFY2(dir.remove(filePath), "Could not remove old capture.");
> +    QVERIFY2(dir.rename(newFilePath, filePath), "Could not rename new 
> capture.");

Why are we replacing the image if it's slightly different?

> ItemCapture.h:61
> +     */
> +    Q_PROPERTY(qreal errorMargin READ errorMargin WRITE setErrorMargin 
> NOTIFY errorMarginChanged)
> +

specify if 1% is

0.01 or 1 ?

> qmltest.cpp:22
> +
> +QUICK_TEST_MAIN_WITH_SETUP(Charts, Setup)
> +

I don't know what QUICK_TEXT_MAIN does, but we probably want to make sure we 
have:

QGuiApplication::setAttribute(AA_DisableHighDpiScaling, true);
QGuiApplication::setDesktopSettingsAware(false);

QQuickWindow::setTextRenderType(the qt rendering)

that should help generating the same thing across machines

REPOSITORY
  R1049 KQuickCharts

REVISION DETAIL
  https://phabricator.kde.org/D27805

To: ahiemstra
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

Reply via email to