Re: Review Request 122861: Micro-optimize LayoutManager
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/ --- (Updated March 10, 2015, 6:05 p.m.) Status -- This change has been marked as submitted. Review request for Plasma. Changes --- Submitted with commit 26596c054dd212ce8ad4333145575414d54b08f2 by Kai Uwe Broulik to branch master. Repository: plasma-desktop Description --- This micro-optimizes the LayoutManager by: - using Array and Object literals rather than new Object/Array, and also creating the whole structure at once if applicable - Store end values in for loops rather than calculating them on each iteration - Adjust coding style here and there Diffs - containments/desktop/package/contents/code/LayoutManager.js 14d0dfc Diff: https://git.reviewboard.kde.org/r/122861/diff/ Testing --- Moving applets (especially using Eike's press-and-hold when using it on a touchscreen :P) feels snappier, doesn't print any new warnings on console and seems to work as before. However, I can no longer cause plasmashell to go berserk when moving a small applet ontop of a huge one (eg. small sticky note on wide fuzzy clock) where it desperately tries to find a place and fails. I think the grid size should be based on units somehow, having a 24x24 grid on a high dpi screen also benefits the aforementioned behavior. One surprising discovery I made is that using Qt.point instead of a handcrafted JS Object is one order of magnitude(!) slower. Thanks, Kai Uwe Broulik ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 122861: Micro-optimize LayoutManager
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/#review77215 --- Looks good. I agree with making the cell size DPI-derived, and I was also planning to drop using iconSize as the name for it because it's rather confusing in the now-combined Desktop/FolderView codebase. - Eike Hein On March 8, 2015, 9:08 p.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/ --- (Updated March 8, 2015, 9:08 p.m.) Review request for Plasma. Repository: plasma-desktop Description --- This micro-optimizes the LayoutManager by: - using Array and Object literals rather than new Object/Array, and also creating the whole structure at once if applicable - Store end values in for loops rather than calculating them on each iteration - Adjust coding style here and there Diffs - containments/desktop/package/contents/code/LayoutManager.js 14d0dfc Diff: https://git.reviewboard.kde.org/r/122861/diff/ Testing --- Moving applets (especially using Eike's press-and-hold when using it on a touchscreen :P) feels snappier, doesn't print any new warnings on console and seems to work as before. However, I can no longer cause plasmashell to go berserk when moving a small applet ontop of a huge one (eg. small sticky note on wide fuzzy clock) where it desperately tries to find a place and fails. I think the grid size should be based on units somehow, having a 24x24 grid on a high dpi screen also benefits the aforementioned behavior. One surprising discovery I made is that using Qt.point instead of a handcrafted JS Object is one order of magnitude(!) slower. Thanks, Kai Uwe Broulik ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 122861: Micro-optimize LayoutManager
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/#review77218 --- Ship it! Ship It! - Sebastian Kügler On March 8, 2015, 9:08 p.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/ --- (Updated March 8, 2015, 9:08 p.m.) Review request for Plasma. Repository: plasma-desktop Description --- This micro-optimizes the LayoutManager by: - using Array and Object literals rather than new Object/Array, and also creating the whole structure at once if applicable - Store end values in for loops rather than calculating them on each iteration - Adjust coding style here and there Diffs - containments/desktop/package/contents/code/LayoutManager.js 14d0dfc Diff: https://git.reviewboard.kde.org/r/122861/diff/ Testing --- Moving applets (especially using Eike's press-and-hold when using it on a touchscreen :P) feels snappier, doesn't print any new warnings on console and seems to work as before. However, I can no longer cause plasmashell to go berserk when moving a small applet ontop of a huge one (eg. small sticky note on wide fuzzy clock) where it desperately tries to find a place and fails. I think the grid size should be based on units somehow, having a 24x24 grid on a high dpi screen also benefits the aforementioned behavior. One surprising discovery I made is that using Qt.point instead of a handcrafted JS Object is one order of magnitude(!) slower. Thanks, Kai Uwe Broulik ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 122861: Micro-optimize LayoutManager
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/#review77202 --- Looks good to me. It would be good if Eike could also have a look since he's refactoring this code right now, and his refactoring might benefit from the same changes, if not avoid conflicts. One stylistic remark (spread over three lines) inline. containments/desktop/package/contents/code/LayoutManager.js https://git.reviewboard.kde.org/r/122861/#comment53030 I think it's more readable to init j before the loop. containments/desktop/package/contents/code/LayoutManager.js https://git.reviewboard.kde.org/r/122861/#comment53031 same here containments/desktop/package/contents/code/LayoutManager.js https://git.reviewboard.kde.org/r/122861/#comment53032 init j before loop for readability - Sebastian Kügler On March 8, 2015, 9:08 p.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/ --- (Updated March 8, 2015, 9:08 p.m.) Review request for Plasma. Repository: plasma-desktop Description --- This micro-optimizes the LayoutManager by: - using Array and Object literals rather than new Object/Array, and also creating the whole structure at once if applicable - Store end values in for loops rather than calculating them on each iteration - Adjust coding style here and there Diffs - containments/desktop/package/contents/code/LayoutManager.js 14d0dfc Diff: https://git.reviewboard.kde.org/r/122861/diff/ Testing --- Moving applets (especially using Eike's press-and-hold when using it on a touchscreen :P) feels snappier, doesn't print any new warnings on console and seems to work as before. However, I can no longer cause plasmashell to go berserk when moving a small applet ontop of a huge one (eg. small sticky note on wide fuzzy clock) where it desperately tries to find a place and fails. I think the grid size should be based on units somehow, having a 24x24 grid on a high dpi screen also benefits the aforementioned behavior. One surprising discovery I made is that using Qt.point instead of a handcrafted JS Object is one order of magnitude(!) slower. Thanks, Kai Uwe Broulik ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 122861: Micro-optimize LayoutManager
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122861/ --- Review request for Plasma. Repository: plasma-desktop Description --- This micro-optimizes the LayoutManager by: - using Array and Object literals rather than new Object/Array, and also creating the whole structure at once if applicable - Store end values in for loops rather than calculating them on each iteration - Adjust coding style here and there Diffs - containments/desktop/package/contents/code/LayoutManager.js 14d0dfc Diff: https://git.reviewboard.kde.org/r/122861/diff/ Testing --- Moving applets (especially using Eike's press-and-hold when using it on a touchscreen :P) feels snappier, doesn't print any new warnings on console and seems to work as before. However, I can no longer cause plasmashell to go berserk when moving a small applet ontop of a huge one (eg. small sticky note on wide fuzzy clock) where it desperately tries to find a place and fails. I think the grid size should be based on units somehow, having a 24x24 grid on a high dpi screen also benefits the aforementioned behavior. One surprising discovery I made is that using Qt.point instead of a handcrafted JS Object is one order of magnitude(!) slower. Thanks, Kai Uwe Broulik ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
