D17240: add keyboard navigation in simplepaint

Johnny Jazeix noreply at phabricator.kde.org
Thu Nov 29 22:34:18 GMT 2018


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>


More information about the kde-edu mailing list