Fixes and improves insert script dialog features

[Patowhiz]

Addresses comments made on PR #8597.

@africanmathsinitiative/developers this is ready for review.

The control in the Save Data tab looks odd and out of alignment because of the label contained within the custom control (in this case save control) doesn't yet support vertical alignment. This issue has to be solved at that control level. Solving it is not only important for this dialog but for translations as well.

Position settings of saving column are not fully reflected on the script generated because the save control needs to be improved. This can be done in a separate PR as well. Note this was not working before this PR.

@rdstern similar to the changes done in the View and Reorder objects, this PR should add to the directions I documented before in regards to the nature of the data book, its data sheets and objects contained in the data sheets. When working on this I got a sense of feeling that it's about time we changed our dialogs "Data Frame" labels to "Data Sheets". I understand that this requires a team discussion so feel free to entirely ignore this point.

Thanks

[rdstern]

@Patowhiz it is looking very promising.
a) Checked the previous feature that it should go to a script window. It used to go to the log window and that caused problems. It was fixed in @N-thony update. That's ok here too
b) Checked on renaming of the tabs that was done by Antoine. The renaming is still fine. But it used to suggest that rename when Saving. That's lost. Good to re-instate it.
c) Then into Insert. I tried to Common feature first. They seem to work individually, but not both together. I am happy to have one at a time, so could they have radio buttons? That would keep it simple.
d) Could we have a better name. I'd prefer Commands or R Commands? .Can you think of anything better than those?
e) I next tried the examples - It looks fine and seems to work ok, and as before. The preview ignores the new lines. It might be simpler to check, if it included them, but not if that is a big deal.
f) I next tried saving the data frame. Why is there the extra first line here?

And when I press To Script I get a third line with a default comment.

I can't change the comment, so please don't have that.
And the extra line plrv <- () makes running the current script more difficult. Does it do any good?

g) I understand why you got rid of Ok, but I now keep pressing Reset, because it is the one on the left and I'm used to pressing that. I suggest you re-instat the Ok key. (It does the same as To-Script, so I'm happy if you have just the 4 keys, but then move To Script to the left. Or have the 5 keys and have To Script as the first and the last?) I would like to be able to press the first (left-hand side) key!

h) It looks as though everything you insert adds that extra curious line (see f above) and also the extra Comment that needs to be left out..

[Patowhiz]

@rdstern thanks for the review.
Can you countercheck that item (b) is really in the master branch, I'm not seeing that feature. This PR has not changed that addition.

I don't understand item (e), you mean when you type on the preview, any additions are ignore?

I've made changes that address the other points.

[rdstern]

@Patowhiz I show the picture below:

I saved the dataframe and that's line 17 in the script added.
But it has also added that curious line 16, and also 15: namely:

# Dialog: Script
ssp <- () 

Do we need them? If we are going to add a comment, then I'd like the "usual" cntrol for comments in the dialog. Then I can untick and not have it - or tailor it, as we can do in all the other dialogs.

And the ssp starting empty. I am sure that will be ok, once Stephen finishes his improvements, but is it needed?

Second is that you ignored my point g above.

I don't mind that there are only 4 buttons here. I do mind that the left hand one is now Reset, because that's what I keep pressing - and that's annoying, because I then have to start again!

I had this problem immediately, in this testing. Namely I just click on the left hand button at the bottom - which is always the Ok button - except here. I would really like a left-hand button here to click on. Could you move the To Script button to the left, or re-instate the 5th button - and maybe call it To Script. (I don't even mind if we keep it as Ok here, because going to the script is what Ok does here. I do mind that the left button is Reset, because that keeps messing me up!)

My point e) is shown below:

In some ways it is nice and consise that there is no new line after ###Name AMMI ? But perhaps it would be clearer if it were essentially the same look as when imported?

[Patowhiz]

@rdstern thanks for the review.

I have now returned the ok button. You raised very valid points that I didn't see before (user consistency in being used to something).

I'm not getting the comment on my side. I'm surprised how you are able to get it since I've explicitly disabled it.

Line ssp <- () is being added to show what or where the ssp parameter is coming from. Do you still want me to remove it?

I don't understand what you mean by same look as imported.

In some ways it is nice and consise that there is no new line after ###Name AMMI ? But perhaps it would be clearer if it were essentially the same look as when imported?

Note if a user wanted the insert dialog to insert with a new line before the script, they can enter the new line in the script window before opening the dialog or the preview text in the dialog. I prefer letting this decision be made by the user using this dialog.

[rdstern]

This is looking nice now.
a) The extra comment line has gone now.
b) @lloyddewit That extra line below ssp <- ( ) is maybe better code, but I currently comment it out, so your "system" runs easily. I assume this will be ok before too long? Can you confirm.
c) I am happy the Ok is now back again.
d) The renaming of a tab (one of the new features before) doesn't suggest that name when you save. That's not enough to stop an approval.
e) Also, for the future, I would like just a few more commands in the Commands tab. I have 2 in mind initially. First is to put the data from a defined data frame into a pivot table, and the second is to define variables as factors.

I suggest @N-thony could check and merge. He is away for a few days, but returns next week.

f <- system.file("external/ssp.csv", package="agricolae")
ssp<-read.csv(f)
#ssp <- ()
data_book$import_data(data_tables=list(ssp=ssp))

[Patowhiz]

@rdstern the extra line added is not because of @lloyddewit . I've deliberately implemented it that way. I have explained why I added it above. And also asked you in light of my explanations if you still find it irrelevant.

[Patowhiz]

@rdstern thanks for more suggestions in regards to this. I'll recommended less and less standard R commands in this dialog. This is a dialog that I think is for users ready to transition to using R through code. I'd lean towards pitching it as a dialog that helps R users learn the databook "api" as a package. This will enable them use R-Instat in a powerful way than R Studio, considering they won't have to write boilerplate code in regards to managing multiple data frames, data outputs, metadata etc. Which makes me wonder whether we are even naming this dialog correctly.

I'd be interested in understanding the criteria you are leaning on of choosing the standard R functions/scripts to include or exclude. Once I understand that criteria then I'd lean towards another dialog that is suited for such purpose.

[Patowhiz]

@lloyddewit you are obsoletely correct. @rdstern line ssp <- () is a dynamically added placeholder. I explained the intention above and asked you whether you still think we should remove it. Refer above to my explanations as to why I added it.

[rdstern]

Thanks @lloyddewit and @Patowhiz - let's remove it.

[lloyddewit]

Thanks @lloyddewit and @Patowhiz - let's remove it.

Or could we replace it with a comment that tells the user that they need to add soemting at this point in the script?

[Patowhiz]

@rdstern as discussed online, I have made changes that add comment descriptions for the scripts produced (excluding Get examples).

We have also agreed that item (b) is also not in the master branch. So can be done in a separate PR.

[rdstern]

@Patowhiz you seem to have gone overboard on the comments now! When I add a file it is now all comments!

You also seem to have changed the get_data_frame command. It doesn't work now.

[Patowhiz]

@rdstern not really.

Kindly note, as stated in my above comment I didn't add any comments for the Get Example tab. I presume those examples have enough comments already. Any comments you get are being retrieved from the package. I have also maintained the initial implementation that is now in the master in regards to retrieving the examples from the packages.

Comments have only been added for Save Data, Get Data and Commands tabs.

For data_book$get_data_frame() function to work, the passed name should match a data frame that exists in the data book. Notice the error displayed in your above error message box.

[rdstern]

@Patowhiz I tried the same example with the merged version and that is fine:

And when I load the example of alwan lamb I (correctly) get the following with the merged version:

Note your ##D from line 10 in your version above that is absent here.

[Patowhiz]

@rdstern thanks for raising the double ##D. I have now resolved the issue.

[rdstern]

@Patowhiz thanks for the ##D stuff.

I am still not able to save the data frame, i.e. transfer it to R-Instat. I assume that's save data? So what do I do to put ssp into a new R-Instat databook? It used to work.

[Patowhiz]

Damn! Yes, that's a wrong command. I'll fix this.

[Patowhiz]

@rdstern I have now fixed the commands

[rdstern]

@Patowhiz great. I am approving.

However, I found a problem that it would be nice if the Save > Data Frame could cope with.

The new feature we don't yet allow for, is when the data is not a single data frame. We now cope with this in the Import from library. Could we have an extended version of the command to add to the script, when we need it in the code?

I used a new package, called gosset - I'll explain the name when you visit!

This had the following line:

data("kenyachoice", package = "gosset")
# Save data frame "kenyachoice"

data_book$import_data(data_tables=list(kenyachoice=kenyachoice))
data_book$import_data(data_tables=lapply(X=kenyachoice, FUN=data.frame))

Your function is the first import_data and doesn't work when there are multiple dataframe. When I copy the last line from the Import From Library it imports the 2 data frames fine!

I am approving, but wonder if this extra option could be added? It could be a new pull request I suppose?

In the figure above I wonder if there could be 2 radio buttons. The default is as shown, then there is a second which says Multiple Data Frames. (It is a nice simple example of lapply!)

[rdstern]

@Patowhiz I now suggest the example above could be done before merging. It is our nice first example of adding a loop.
Then, when you are in England I would like to discuss, also with @lilyclements how we extend this feature to add one way we facilitate adding loops in R-Instat.

I refer here to the pull requests for reference:

a) Multiple (Excel) files. This is issue #8521. I now suggest that the added line that is needed could become a third option on the existing Save Data tab with the Data Frame option!
b) The loops referred to in the reviews are:

1. Doing (manipulation) things for multiple columns when the dialog only works for a single column. (So we need to select multiple columns. We have a select dialog!)
2. Doing (manipulation) things for multiple levels of a factor when the simple manipulation is for a single level. (We have a dialog to choose selected levels of a factor and make a set of filters from them.
3. Doing multiple analyses. This could either imply 1 or 2 above.

I suggest we add a Loop tab to the dialog. This has Factor Levels and Variables buttons. It then calls on either the Filter control, or the Select control to provide the list of factor levels or variables.

Then I don't know. My R isn't good enough. Do we use lapply, or purrr or foreach. All are installed in R-Instat already. This stuff is referred to in issues #4386, #4228, #3928 and #4445.

Very exciting.

[N-thony]

@rdstern it looks good, just noticed a small point when navigating from Save Data or Commands, it keeps reseting and the Package comboxes don't have the default value.
For the suggested tab name in the save dialogue, this line code is the one that was missing

If you are happy the way it is, we can get it merged unless the code above is added?

[Patowhiz]

@N-thony, would you be willing to include that particular line in the script window through a separate PR? Additionally, could you verify whether the line is currently absent from the master branch?

[N-thony]

@N-thony, would you be willing to include that particular line in the script window through a separate PR? Additionally, could you verify whether the line is currently absent from the master branch?

@Patowhiz this line of code is missing in the merged version, as it is just one line I think that could have been easier to add it here rather creating a new PR about it. Once it is done, we can merge this PR. Thanks!

[rdstern]

@Patowhiz I like the change. I tested with the gosset package (which I had to install and which is becoming important for us) and the function ptpermute. It uses the kenyachoice dataset, which is interesting in itself, and which has 2 data frames. Wgen I untick single, it gives the correct command to import the 2 data frames into the R-Instat data book.

@N-thony can you check again. It would be great to get this loaded.
Could you also add the code to suggest the renamed text, when a tab is saved?