D17240: add keyboard navigation in simplepaint

[Johnny Jazeix]

jjazeix added a comment.


  Thank you for the PR, I did some comments

INLINE COMMENTS

> PaintCursor.qml:1
> +/* GCompris - PaintItem.qml
> + *

update the filename

> Simplepaint.qml:44
> +
> +        Keys.onUpPressed:{
> +

the coding rules incites to add a space before the {
In the if condition too (if(true) { ... )

> Simplepaint.qml:48
> +                if (--items.current_color<0){
> +                    items.current_color=items.colors.length-1
> +                }

can you add spaces before and after '='?

> Simplepaint.qml:99
> +        function changeTab(){
> +            isColorTab=isColorTab?false:true
> +            if(isColorTab){

isColorTab = !isColorTab

> Simplepaint.qml:146
>              property var colors: bar.level < 10 ? Activity.colorsSimple : Activity.colorsAdvanced
> -            property string colorSelector: colors[0]
> +            property var current_color:0
> +            property string colorSelector: colors[current_color]

use an "int" if you already know the type. var is slower

> Simplepaint.qml:349
>              }
> -            onReloadClicked: Activity.initLevel()
> -            onPreviousLevelClicked: Activity.previousLevel()
> -            onNextLevelClicked: Activity.nextLevel()
> +            onReloadClicked: {Activity.initLevel(); background.refreshCursor()}
> +

I think you can directly put the background.refreshCursor() in the initLevel().
In this case, you don't need to call it on onPreviousLevelClicked and onNextLevelClicked as they call the initLevel()

REPOSITORY
  R2 GCompris

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

To: jjazeix, #gcompris_improvements
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20181129/cc3ac996/attachment.html>