-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added new features to script window #7871
Added new features to script window #7871
Conversation
…ated with the designer
@rdstern @Patowhiz |
@rdstern
If you agree, then I think this would be a good time to merge. I would implement the next improvements in a new PR. Possible next improvements:
Do you have a preference regarding which I should implement first (I can provide time estimates if needed)? If you want to compare against RStudio, then the RStudio editor functionality is described below. https://support.rstudio.com/hc/en-us/articles/200484448-Editing-and-Executing-Code-in-the-RStudio-IDE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lloyddewit this is looking very nice indeed. I have searched for something to complain about, but it seems to work fine!
@lloyddewit I don't have strong views about the order of the items in the new pull request. So very happy for you to decide on the most efficient logical order. |
This is just a note for myself that I could test the script window with code from issue #7889. |
End Using | ||
End Sub | ||
|
||
Private Sub mnuCopy_Click(sender As Object, e As EventArgs) Handles mnuCopy.Click | ||
If txtScript.SelectionLength > 0 Then | ||
If txtScript.SelectedText.Length > 0 Then | ||
CopyText() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
txtScript.Copy() on line 204? to make it consistent with line 211 and get rid of CopyText()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I understand your point.
Copytext()
is also called by frmMain
. Therefore, we cannot delete it.
If we don't call CopyText()
on line 204, then we also need to call EnableDisableButtons()
which adds an extra line of duplicated code.
So on balance, I'll leave the above code unchanged. I hope that's OK.
This PR is draft, not yet ready to review.
Fixes #6645
Fixes #6541
Fixes (partly) #4932
Fixes (potentially):