Re: Review Request: Game of Life Plasmoid -- Cleanup and reflection/density feature addition
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5027/ --- (Updated 2010-08-23 19:12:21.184543) Review request for Plasma. Changes --- The updated diff is built on top of the bugfix which was submitted as 1166819 in trunk (http://reviewboard.kde.org/r/5055/) Also included in the updated diff are member variable renamings (prepending m_ to member variable names.) Summary (updated) --- Update (8/23/10): The patch adds two new features to the life applet: game board reflection (the user can choose to generate initial populations that are symmetrical about the horizontal and/or vertical axes,) and user-configurable population density (the user can choose what approximate percentage of cells will be alive in the initial population.) With the addition of the new configuration options, the configuration UI was also updated so that using tab to scroll through options would be consistent (top to bottom.) This submit also includes member variable renamings (prepending m_ to member variable names.) Diffs (updated) - /trunk/KDE/kdeplasma-addons/applets/life/life.h 1166777 /trunk/KDE/kdeplasma-addons/applets/life/life.cpp 1167084 /trunk/KDE/kdeplasma-addons/applets/life/lifeConfig.ui 1166777 Diff: http://reviewboard.kde.org/r/5027/diff Testing --- Various game board sizes were tested (odd and even heights and widths, square and non-square.) The configuration dialog was opened several times and tested to confirm tab order. Various population densities were tested, including 0% (confirmed no cells were alive) and 100% (confirmed that all cells were alive.) All combinations of vertical/horizontal/no reflection were tested at odd and even heights and widths, square and non-square. Tests consisted of setting the proper configuration options, then watching the board for a few generations and confirming that no crashes occurred and that all cells appeared to live and die properly. Screenshots --- Updated Configuration Dialog http://reviewboard.kde.org/r/5027/s/481/ Board Using Vertical and Horizontal Reflection http://reviewboard.kde.org/r/5027/s/482/ Thanks, obby ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Game of Life Plasmoid -- Cleanup and reflection/density feature addition
On 2010-08-15 07:56:46, Marco Martin wrote: that'a a good feature. would be possible separe in two commits, bugfix and feature? the bugfix one (and only that) should be backported to 4.5 I definitely can split this up into two commits. Should I include the variable renaming below (prepending m_ to member variables) in the bugfix submit? Or should I include them as part of the feature submit (or a new commit altogether?) Also, I have applied for a kdesvn account, but currently have not heard back. I can upload an updated diff here for the feature, and an additional diff for the bugfix, but I'll need a kind reviewer to commit each after they pass review. Thanks! - obby --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5027/#review7060 --- On 2010-08-14 22:51:07, obby wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5027/ --- (Updated 2010-08-14 22:51:07) Review request for Plasma. Summary --- This patch fixes a bug wherein the very last cell in the board would never come alive. Also, the configuration UI was updated so that using tab to scroll through options would be consistent (top to bottom.) The patch also adds to new features: game board reflection (the user can choose to generate initial populations that are symmetrical about the horizontal and/or vertical axes,) and user-configurable population density (the user can choose what approximate percentage of cells will be alive in the initial population.) Diffs - /trunk/KDE/kdeplasma-addons/applets/life/life.h 1163310 /trunk/KDE/kdeplasma-addons/applets/life/life.cpp 1163310 /trunk/KDE/kdeplasma-addons/applets/life/lifeConfig.ui 1163310 Diff: http://reviewboard.kde.org/r/5027/diff Testing --- Various game board sizes were tested (odd and even heights and widths, square and non-square.) The configuration dialog was opened several times and tested to confirm tab order. Various population densities were tested, including 0% (confirmed no cells were alive) and 100% (confirmed that all cells were alive.) All combinations of vertical/horizontal/no reflection were tested at odd and even heights and widths, square and non-square. Tests consisted of setting the proper configuration options, then watching the board for a few generations and confirming that no crashes occurred and that all cells appeared to live and die properly. Screenshots --- Updated Configuration Dialog http://reviewboard.kde.org/r/5027/s/481/ Board Using Vertical and Horizontal Reflection http://reviewboard.kde.org/r/5027/s/482/ Thanks, obby ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Game of Life Plasmoid -- Cleanup and reflection/density feature addition
On 2010-08-15 07:56:46, Marco Martin wrote: that'a a good feature. would be possible separe in two commits, bugfix and feature? the bugfix one (and only that) should be backported to 4.5 obby wrote: I definitely can split this up into two commits. Should I include the variable renaming below (prepending m_ to member variables) in the bugfix submit? Or should I include them as part of the feature submit (or a new commit altogether?) Also, I have applied for a kdesvn account, but currently have not heard back. I can upload an updated diff here for the feature, and an additional diff for the bugfix, but I'll need a kind reviewer to commit each after they pass review. Thanks! Marco Martin wrote: yes, the variable renaming should be in the feature one. I hope your account gets approved soon. in the meantime i could do it if it would still not have been approved. I've opened http://reviewboard.kde.org/r/5055/ for the bugfix. Once that is submitted, I'll have quite an easier time creating an updated diff for just the feature addition here. I'll update the description here after the bugfix is in as well. - obby --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5027/#review7060 --- On 2010-08-14 22:51:07, obby wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5027/ --- (Updated 2010-08-14 22:51:07) Review request for Plasma. Summary --- This patch fixes a bug wherein the very last cell in the board would never come alive. Also, the configuration UI was updated so that using tab to scroll through options would be consistent (top to bottom.) The patch also adds to new features: game board reflection (the user can choose to generate initial populations that are symmetrical about the horizontal and/or vertical axes,) and user-configurable population density (the user can choose what approximate percentage of cells will be alive in the initial population.) Diffs - /trunk/KDE/kdeplasma-addons/applets/life/life.h 1163310 /trunk/KDE/kdeplasma-addons/applets/life/life.cpp 1163310 /trunk/KDE/kdeplasma-addons/applets/life/lifeConfig.ui 1163310 Diff: http://reviewboard.kde.org/r/5027/diff Testing --- Various game board sizes were tested (odd and even heights and widths, square and non-square.) The configuration dialog was opened several times and tested to confirm tab order. Various population densities were tested, including 0% (confirmed no cells were alive) and 100% (confirmed that all cells were alive.) All combinations of vertical/horizontal/no reflection were tested at odd and even heights and widths, square and non-square. Tests consisted of setting the proper configuration options, then watching the board for a few generations and confirming that no crashes occurred and that all cells appeared to live and die properly. Screenshots --- Updated Configuration Dialog http://reviewboard.kde.org/r/5027/s/481/ Board Using Vertical and Horizontal Reflection http://reviewboard.kde.org/r/5027/s/482/ Thanks, obby ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Game of Life Applet - fix to off-by-one in array traversal
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5055/ --- Review request for Plasma and Marco Martin. Summary --- This patch fixes a off-by-one error which would lead to the last cell of the array never coming alive. Also, parentheses were added to disambiguate some arithmetic expression and enforce operator precedence. Diffs - /trunk/KDE/kdeplasma-addons/applets/life/life.cpp 1163310 Diff: http://reviewboard.kde.org/r/5055/diff Testing --- Started applet, determined that the state of last cell in the array (bottommost rightmost cell) was calculated properly, and came alive when appropriate. Thanks, obby ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Game of Life Plasmoid -- Cleanup and reflection/density feature addition
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5027/ --- Review request for Plasma. Summary --- This patch fixes a bug wherein the very last cell in the board would never come alive. Also, the configuration UI was updated so that using tab to scroll through options would be consistent (top to bottom.) The patch also adds to new features: game board reflection (the user can choose to generate initial populations that are symmetrical about the horizontal and/or vertical axes,) and user-configurable population density (the user can choose what approximate percentage of cells will be alive in the initial population.) Diffs - /trunk/KDE/kdeplasma-addons/applets/life/life.h 1163310 /trunk/KDE/kdeplasma-addons/applets/life/life.cpp 1163310 /trunk/KDE/kdeplasma-addons/applets/life/lifeConfig.ui 1163310 Diff: http://reviewboard.kde.org/r/5027/diff Testing --- Various game board sizes were tested (odd and even heights and widths, square and non-square.) The configuration dialog was opened several times and tested to confirm tab order. Various population densities were tested, including 0% (confirmed no cells were alive) and 100% (confirmed that all cells were alive.) All combinations of vertical/horizontal/no reflection were tested at odd and even heights and widths, square and non-square. Tests consisted of setting the proper configuration options, then watching the board for a few generations and confirming that no crashes occurred and that all cells appeared to live and die properly. Screenshots --- Updated Configuration Dialog http://reviewboard.kde.org/r/5027/s/481/ Board Using Vertical and Horizontal Reflection http://reviewboard.kde.org/r/5027/s/482/ Thanks, obby ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Fix to game of life applet to avoid accessing memory outside bounds of array.
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4361/ --- Review request for Plasma. Summary --- This patch fixes an off-by-one error which led to accessing memory just outside of the cells array. It also kept the first character in the cells array from ever being displayed on the board properly, and a bunch of crazy math to keep the rules of the game intact. (The last cell displayed was reflecting the state of memory outside of the cells array.) Diffs - /trunk/KDE/kdeplasma-addons/applets/life/life.h 1138038 /trunk/KDE/kdeplasma-addons/applets/life/life.cpp 1138038 Diff: http://reviewboard.kde.org/r/4361/diff Testing --- Aside from the initial test cases which were used to confirm the bug (hard coding one char in the cells array to always be 1 and moving it around to find the bounds of the display,) I also generated a few other board states manually using examples found here: http://en.wikipedia.org/wiki/Conway%27s_Game_of_Life Blinker and Toad were two that were used to test. Thanks, obby ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
