rebuild mosaic multiple dataset
ClosedPublic

Authored by dekumar on Feb 22 2020, 8:14 PM.

Details

Diff Detail

Repository
R2 GCompris
Branch
arcpatch-D27583_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26110
Build 26128: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
dekumar added inline comments.Mar 8 2020, 7:08 PM
src/activities/mosaic/resource/1/Data.qml
30

I have just updated the diff by adding layout rows and columns in form of 2d array. Let me know if this is all good or I shall make use of 4 different objects.
Thanks!

timotheegiet requested changes to this revision.Mar 8 2020, 7:24 PM

As discussed on IRC, the layout has some issues, and can be improved in several cases...
I suggest to use some states to handle all possible situations, making the best use of the screen area available in any case.

This revision now requires changes to proceed.Mar 8 2020, 7:24 PM

More specifically, layout issues are (as said on irc, but noting those here for reference):

-on first dataset levels, in vertical mode, the areas go beyond the screen
-on first dataset levels, in horizontal mode at some sizes, the items overflow of their areas
-if screen ratio is close to square (but still considered horizontal), there is a big empty area (visible on lvl 6 to 9, and same on lvl 13 to 21)

I'm wondering if we should not have the image names in the dataset too so later we can choose which images to display

src/activities/mosaic/Mosaic.qml
69

it seems to be a string?

144

why (column.horizontal ? 1 : 1)?

src/activities/mosaic/resource/1/Data.qml
34

it's not intuitive, we don't know what it means or does. Can you rename it?

dekumar added inline comments.Mar 9 2020, 7:14 PM
src/activities/mosaic/Mosaic.qml
69

It's a typo. I would fixed it.

144

I am making changes to the layout using different states. This issue would be fixed when I shall update the diff.

src/activities/mosaic/resource/1/Data.qml
34

Yeah, sure. What do you think of renaming is as "multipledataType" ??

jjazeix added inline comments.Mar 10 2020, 7:13 AM
src/activities/mosaic/resource/1/Data.qml
34

The issue is about data1, data2. What does it mean? Put yourself in the place of someone that does not know the code, starts to read it and see: we are using "data1". How do you interpret that?

dekumar added inline comments.Mar 10 2020, 7:50 AM
src/activities/mosaic/resource/1/Data.qml
34

Sure, I would rename it.

I'm wondering if we should not have the image names in the dataset too so later we can choose which images to display

Can you elaborate a bit more about it? Are you talking about the images in mosaic.js file?

I'm wondering if we should not have the image names in the dataset too so later we can choose which images to display

Can you elaborate a bit more about it? Are you talking about the images in mosaic.js file?

Yes, should we allow the possibility to the creator of the dataset to specify which images he wants? May be useful later for teachers or customisation. May be too much too.
@timotheegiet @echarruau does it make sense?

timotheegiet added a comment.EditedMar 10 2020, 1:35 PM

indeed, it would make sense to specify the image names in the dataset as you suggest.

dekumar updated this revision to Diff 77353.Mar 10 2020, 2:03 PM
  • fixed layout issues
  • Fixed question layout
  • Fixed all layout
  • Made all the requested changes

Tested, the layout can still be improved a lot...
Though as it is not really part of the dataset changes, please keep it as it is, and I'll work on it after your changes are merged.

Still needed is to move the images names as part of the datasets, as suggested by Johnny.

jjazeix added inline comments.Mar 10 2020, 4:57 PM
src/activities/mosaic/resource/1/Data.qml
34

Same issue. We still don't know what multipleDataset1 means

timotheegiet added inline comments.Mar 10 2020, 5:03 PM
src/activities/mosaic/resource/1/Data.qml
34

Though he added that variable for layout changes, but as I will anyway do a big rework of the layout after, which will probably not need that variable anymore, maybe it can be kept like this for now... (and if I still need a variable depending on datasets, I'll rename it properly depending on why it's actually needed...)

jjazeix added inline comments.Mar 11 2020, 6:39 AM
src/activities/mosaic/resource/1/Data.qml
34

For the sake of the exercice of naming variables cleanly I'd prefer @dekumar to really think of a good name. Even if it will removed later or you can change it, it's important for him to be able to name correctly variables.

Still needed is to move the images names as part of the datasets, as suggested by Johnny.

I would complete this work as part of the multiple dataset and update the diff soon.

dekumar added inline comments.Mar 11 2020, 12:04 PM
src/activities/mosaic/resource/1/Data.qml
34

@jjazeix Yeah, I would think of naming good variable names to it. As I agree with you that it is the part of my learning process and would help me a lot in future too.
Thanks:)

Tested, the layout can still be improved a lot...
Though as it is not really part of the dataset changes, please keep it as it is, and I'll work on it after your changes are merged.

Thanks a lot for testing @timotheegiet . I would keep it as with the changes to layout I have done for now. Shall I open a new task for this purpose?

@dekumar I just opened a task for it T12809

src/activities/mosaic/resource/1/Data.qml
34

@jjazeix That's right, ok.

I'm wondering if we should not have the image names in the dataset too so later we can choose which images to display

Can you elaborate a bit more about it? Are you talking about the images in mosaic.js file?

Yes, should we allow the possibility to the creator of the dataset to specify which images he wants? May be useful later for teachers or customisation. May be too much too.
@timotheegiet @echarruau does it make sense?

Hi,
It does really make sens. We need to think about the "step after". We may have in mind, (even if we do not do it at the end) that our server should allow to customise activities. The server could allow adding sublevels then presenting the attributes to fill. The variable therefore must be self explanatory, and we may have to add comments in front of the attributes to tell what they are and how to fill them. eg: //layout: describes the way the cells are displayed, eg: "layout": [ [6,4], [12,2] ],
In my case as programmer, thinking about the final user would force me to replace for example nbItems by nbOfCells which is clearer for the final user.
Here as final user, for example I do not know what multipleDatasetType is neither multipleDataset2.

There are mistakes in the multipledataset option text, see my online comments.

We could split multipledataset 2 2 in two different levels according to the number of items.

We could add an additional level but this includes quite a lots of change.
For this we need to have a different size between the model and the target. Let me explain you why.
In the current mosaic (single line option) children do not need to recall the order of the elements. They simply have to recognise one elements and put the same under. This is not what we are aiming at :)
Solution: have a smaller model that the target.

I recognize that the layout could be better if it would be centered. Nonetheless well done :) These are just details.

To sum up
multipledata1: as it is now but centered
multipledata2: as it is now but with a smaller model than the target (should be centered and we need to add an attribute to the multipledata)
multipledata3: first half of multipledata2
multipledata4: second half of multipledata2

src/activities/mosaic/resource/2/Data.qml
25

"Rebuild" not rebuid. Anyway the we could have a shorter description:
Items are on a single line.
Items on multiple lines.

I do not understand the multipledata variable you use.

src/activities/mosaic/Mosaic.qml
128–130

I don't understand this multipledataset1 variable.
Is it related to the device layout (portrait/landscape) or to the layout targeted 'single/multiple line".

dekumar added a comment.EditedMar 12 2020, 1:15 PM

I'm wondering if we should not have the image names in the dataset too so later we can choose which images to display

Can you elaborate a bit more about it? Are you talking about the images in mosaic.js file?

Yes, should we allow the possibility to the creator of the dataset to specify which images he wants? May be useful later for teachers or customisation. May be too much too.
@timotheegiet @echarruau does it make sense?

Hi,
It does really make sens. We need to think about the "step after". We may have in mind, (even if we do not do it at the end) that our server should allow to customise activities. The server could allow adding sublevels then presenting the attributes to fill. The variable therefore must be self explanatory, and we may have to add comments in front of the attributes to tell what they are and how to fill them. eg: //layout: describes the way the cells are displayed, eg: "layout": [ [6,4], [12,2] ],
In my case as programmer, thinking about the final user would force me to replace for example nbItems by nbOfCells which is clearer for the final user.
Here as final user, for example I do not know what multipleDatasetType is neither multipleDataset2.

@echarruau Hi, Thanks for the review. I would rename the variable with good names and add comments too as described by you.

src/activities/mosaic/Mosaic.qml
128–130

@echarruau Hi, It is related with the targeted layout "single/multipleLine". I have made the use the states in the code to change different property in case of both of the multiple dataset.

src/activities/mosaic/resource/2/Data.qml
25

Sure, I would update it.

There are mistakes in the multipledataset option text, see my online comments.

We could split multipledataset 2 2 in two different levels according to the number of items.

We could add an additional level but this includes quite a lots of change.
For this we need to have a different size between the model and the target. Let me explain you why.
In the current mosaic (single line option) children do not need to recall the order of the elements. They simply have to recognise one elements and put the same under. This is not what we are aiming at :)
Solution: have a smaller model that the target.

I recognize that the layout could be better if it would be centered. Nonetheless well done :) These are just details.

To sum up
multipledata1: as it is now but centered
multipledata2: as it is now but with a smaller model than the target (should be centered and we need to add an attribute to the multipledata)
multipledata3: first half of multipledata2
multipledata4: second half of multipledata2

@echarruau Regarding the layout rework, I had a discussion with @timotheegiet earlier as he going to do the rework when the multiple datasets are merged. As mentioned about the splitting the multiple datasets I would make that changes and update the diff.
You can find the task related with the changes to layout of the activity here and add comments here regarding the layout so it would be good to refer later https://phabricator.kde.org/T12809
Thanks :)

dekumar updated this revision to Diff 77806.Mar 17 2020, 8:01 AM
  • Added all the multiple dataset and renamed the variable

@jjazeix @echarruau @timotheegiet I have done all the requested changes expect adding a OK button. I would add it too and update the diff soon.
Thanks :)

timotheegiet accepted this revision.Mar 29 2020, 4:39 PM

After checking again the discussion here and on the tasks, and testing again the patch, I think it is good to go.
Adding the optional OK button can be done later, it is not really part of the dataset changes.

And for the remaining work, that is all related to layout, so I will work on it after merging the patch.

This revision is now accepted and ready to land.Mar 29 2020, 4:39 PM
dekumar updated this revision to Diff 79319.Apr 4 2020, 3:45 PM
  • Updated to new multiple dataset style

We should have different objectives for all multiple datasets too, for now 1-2 and 3-4 have the exact same and it's not good, users need to know the difference between the datasets.
Also, you should probably add the variable that will be used to scale the model with a minimal handling, even if @timotheegiet is going to rework after.

src/activities/mosaic/resource/3/Data.qml
25

difficulty should be just below objective

src/activities/mosaic/resource/4/Data.qml
26

difficulty should be just below objective

We should have different objectives for all multiple datasets too, for now 1-2 and 3-4 have the exact same and it's not good, users need to know the difference between the datasets.
Also, you should probably add the variable that will be used to scale the model with a minimal handling, even if @timotheegiet is going to rework after.

@jjazeix I remember as told by @echarruau to give the objective short and same. And regarding the use of extra variable @timotheegiet told me to keep it as it is now when he would work on the rework of layout he won't required any variable to make all the changes.

Yes I said I was not sure I would need a variable, but actually as it will be a scaling depending on dataset content, it's better indeed to have a variable to define this mode within the dataset.

jjazeix added inline comments.Apr 12 2020, 9:09 AM
src/activities/mosaic/resource/1/Data.qml
64

you can remove this line break

83

missing "" and indentation of almost all "modelDisplayLayout" is not good

dekumar updated this revision to Diff 79915.Apr 12 2020, 10:36 AM
  • All requested changes made
jjazeix added inline comments.Apr 12 2020, 8:47 PM
src/activities/mosaic/resource/2/Data.qml
62

why using the same variable for 2 differents use cases?
they are 2 different features, so let's separate them in two different variables.
If each time we add an option, we have to rename: modelDisplayLayout to singleColumnReducedNewFeatureAndAnotherOne and in the code do
if(modelDisplayLayout == "single..." || "single..." || ...) and somewhere else if(modelDisplayLayout == "...Reduced..." || ...) it is not readable or evolutive.

dekumar added inline comments.Apr 13 2020, 2:02 AM
src/activities/mosaic/resource/2/Data.qml
62

@jjazeix Ah, I see as I thought that we use to just use four different variables names for same object. I will update the first one. Regarding the second point I have only made the use of two states now as so I handled it using or for now so that Animtim would update later with minimum code deletion. Anyway now I will make the changes for now the first one as to use 4 different variables and the second one to make use of 4 different states for each of the multiple dataset for now.

@jjazeix Once just confirm me regarding the use of 4 different states and should I access the all four variables for different datasets. I am sorry for my small confusion regarding it :)

@jjazeix Once just confirm me regarding the use of 4 different states and should I access the all four variables for different datasets. I am sorry for my small confusion regarding it :)

How would you code the feature of having a smaller grid? Not the whole code, can you tell the basic element?

@jjazeix Once just confirm me regarding the use of 4 different states and should I access the all four variables for different datasets. I am sorry for my small confusion regarding it :)

How would you code the feature of having a smaller grid? Not the whole code, can you tell the basic element?

For each of the three grid namely selector, question, and the answer the height and width value of the grid is setup according to the number of columns and itemWidth defined in the Column item. So by changing the valuesof itemWidth and depending upon the numberofColumns the grid size can be scaled up or down.
Thanks!

jjazeix added a comment.EditedApr 13 2020, 8:40 AM

Use 2 variables:

  • "modelDisplayLayout" either "singleColumn" or "doubleColumn"
  • "scaleGridRatio" which is a float number. Above 1, we will scale down the first, below one the second (@timotheegiet will decide on what we want to do with this variable later or how to handle it).
dekumar updated this revision to Diff 80025.Apr 13 2020, 4:06 PM
  • Updated to new multiple dataset style
  • All requested changes made
  • Made all the rqeuested changes
  • Typo fixed
dekumar updated this revision to Diff 80264.Apr 16 2020, 8:30 AM
  • gridRatio updated
jjazeix added a comment.EditedApr 19 2020, 4:03 PM

We should have different objectives for all multiple datasets too, for now 1-2 and 3-4 have the exact same and it's not good, users need to know the difference between the datasets.
Also, you should probably add the variable that will be used to scale the model with a minimal handling, even if @timotheegiet is going to rework after.

@jjazeix I remember as told by @echarruau to give the objective short and same. And regarding the use of extra variable @timotheegiet told me to keep it as it is now when he would work on the rework of layout he won't required any variable to make all the changes.

I still think objectives should all be different. It makes no sense to propose same objectives but not same content. If there is a difference, users should be able to know about it when choosing the dataset.

Except this, seems ok for me as long as @timotheegiet updates the layout.

Hi,
I can not remember telling that objectives should be the same? Obviously
the objectives have to match the goals to be be achieved. It must be a
misanderstanding.
Emmanuel

Le dim. 19 avr. 2020 à 18:03, Johnny Jazeix <noreply@phabricator.kde.org> a
écrit :

jjazeix added a comment. View Revision
https://phabricator.kde.org/D27583

In D27583#645919 https://phabricator.kde.org/D27583#645919, @dekumar
https://phabricator.kde.org/p/dekumar/ wrote:

In D27583#645436 https://phabricator.kde.org/D27583#645436, @jjazeix
https://phabricator.kde.org/p/jjazeix/ wrote:

We should have different objectives for all multiple datasets too, for now
1-2 and 3-4 have the exact same and it's not good, users need to know the
difference between the datasets.
Also, you should probably add the variable that will be used to scale the
model with a minimal handling, even if @timotheegiet
https://phabricator.kde.org/p/timotheegiet/ is going to rework after.

@jjazeix https://phabricator.kde.org/p/jjazeix/ I remember as told by
@echarruau https://phabricator.kde.org/p/echarruau/ to give the
objective short and same. And regarding the use of extra variable
@timotheegiet https://phabricator.kde.org/p/timotheegiet/ told me to
keep it as it is now when he would work on the rework of layout he won't
required any variable to make all the changes.

I still think objectives should all be different. It makes no sense to
propose same objectives but not same content. If there is a difference,
users should be able to know about it when choosing the dataset.

*REPOSITORY*
R2 GCompris

*BRANCH*
arc2

*REVISION DETAIL*
https://phabricator.kde.org/D27583

*To: *dekumar, GCompris: Improvements, jjazeix, timotheegiet, echarruau
*Cc: *kde-edu, narvaez, apol

@echarruau @jjazeix No problem. I would update the objectives.
Thanks!

dekumar updated this revision to Diff 80933.Apr 22 2020, 7:32 PM
  • Objective Updated

There is a problem in the multipledataset difficulty progression.
We have twice "Items are placed on single line with number of items up to 6" in a row, and it jumps directly to 16 items.
I would prefer to have the second multipledataset set to ""Items are placed on single line with number of items up to 8"
or
"Items are placed on multiple lines with number of items up to 8" if it does not fit.

Also I would prefer a shorter message:

Up to 6 items placed on single line would be enough for me.

There is a problem in the multipledataset difficulty progression.
We have twice "Items are placed on single line with number of items up to 6" in a row, and it jumps directly to 16 items.
I would prefer to have the second multipledataset set to ""Items are placed on single line with number of items up to 8"
or
"Items are placed on multiple lines with number of items up to 8" if it does not fit.

Also I would prefer a shorter message:

Up to 6 items placed on single line would be enough for me.

Thanks for the review @echarruau . I would update the datasets content and the objective as per your comments.

dekumar updated this revision to Diff 81537.Apr 29 2020, 6:01 PM
  • Requested changes made
timotheegiet requested changes to this revision.Apr 29 2020, 7:01 PM

I think Dataset 3 and 4 (and maybe 2) should say "on multiple lines", not "on single line"...

This revision now requires changes to proceed.Apr 29 2020, 7:01 PM
dekumar updated this revision to Diff 81550.Apr 29 2020, 7:46 PM
  • Typo fixed

Just tested. It is ok for me now concerning the multipledataset part.

I have a question about layout. Could we not fit the height of the answer area to the height available on the screen. This would prevent to have very small place when dealing with 8 items in a row.

@echarruau : as I said before: I will redo the layout of the activity after the patch is pushed, to make sure it is optimized as much as possible in any case.

timotheegiet accepted this revision.Apr 30 2020, 1:08 PM

Good for me to merge now.

This revision is now accepted and ready to land.Apr 30 2020, 1:08 PM
jjazeix accepted this revision.May 3 2020, 8:19 AM

I renamed "verticalMode" to "singleColumnMode" because it was weird to have both "horizontal" and "verticalMode" variables

jjazeix closed this revision.May 3 2020, 8:19 AM