Port away from KRandom
ClosedPublic

Authored by nicolasfella on Dec 23 2019, 11:18 PM.

Details

Summary

See T12093

Diff Detail

Repository
R410 KPatience
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nicolasfella created this revision.Dec 23 2019, 11:18 PM
Restricted Application added a reviewer: KDE Games. · View Herald TranscriptDec 23 2019, 11:18 PM
Restricted Application added a subscriber: kde-games-devel. · View Herald Transcript
nicolasfella requested review of this revision.Dec 23 2019, 11:18 PM
nicolasfella edited the summary of this revision. (Show Details)Dec 23 2019, 11:18 PM

Thanks for the patch! It looks fine at first glance, but I noticed that when you deal the first game after the game variant carousel ([kpat start] → Freecell) the deal index in the window's title is always "1". It doesn't happen with the current git master ( 96de699e8fe1b6c56881b29761384b5a600ad6ab ).

shlomif requested changes to this revision.Jan 2 2020, 2:07 PM

Here is a fixed startRandom():

void MainWindow::startRandom()
{
    const auto seed = QRandomGenerator::global()->generate();
    const int signed_seed = (int)(seed & (~(((quint32)1) << 31)));
    startNew(signed_seed);
}
This revision now requires changes to proceed.Jan 2 2020, 2:07 PM
coates added a subscriber: coates.Jan 8 2020, 1:45 PM

Using QRandomGenerator, but keeping RAND_MAX is really only a half port. Please, use QRandomGenerator's convenient generation functions.

dealer.cpp
792
qreal randomExp = qMin<qreal>( -log( 1 - QRandomGenerator::global()::generateDouble() ) / 4, 1 );
mainwindow.cpp
422
KCardTheme theme = themes.at( QRandomGenerator::global()->bounded( themes.size() ) );
spider.cpp
406
qreal x = rect.left() + QRandomGenerator::global()::bounded(rect.width() - deck()->cardWidth());
qreal y = rect.top() + QRandomGenerator::global()::bounded(rect.height() - deck()->cardHeight());
shlomif updated this revision to Diff 73720.Jan 16 2020, 4:28 PM
  • fix the non-randomised deal problem.
  • Use specialized QRandomGenerator methods.
coates added inline comments.Jan 16 2020, 4:57 PM
mainwindow.cpp
411

Personally, I think the following would be more readable, but at minimum a comment explaining the bit fiddling would be appropriate.

const int gameNumber = int(QRandomGenerator::global()->bounded(quint32(0), quint32(INT_MAX) + 1));
startNew(gameNumber);
shlomif updated this revision to Diff 73725.Jan 16 2020, 5:44 PM
  • Readability improvement per coates' advice.
coates accepted this revision.Jan 17 2020, 12:58 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 19 2020, 10:18 AM
Closed by commit R410:e6b2c641ff2e: Port away from KRandom (authored by nicolasfella, committed by shlomif). · Explain Why
This revision was automatically updated to reflect the committed changes.