Skip to content
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

Moved the log window to the first tab in the script window #8191

Merged
merged 20 commits into from Mar 31, 2023

Conversation

lloyddewit
Copy link
Contributor

Fixes #8190

@lloyddewit lloyddewit marked this pull request as draft March 4, 2023 11:02
@lloyddewit
Copy link
Contributor Author

Ongoing, nothing to test yet

@lloyddewit
Copy link
Contributor Author

lloyddewit commented Mar 19, 2023

@rdstern I implemented the log tab in the script window. I now need to test it thoroughly. Before I test, there's one thing I'd still like to clarify.

There are currently four ways of opening the log/script window: two view menu buttons and two toolbar buttons. You originally requested that we keep all four options, and each option would open the log/script window. The only difference would be that the log buttons would open the log tab, and the script buttons would open one of the script tabs.

I started to implement this but it fast becomes confusing for the user, and also adds extra compexity/risk to the code. For example, when should the buttons be ticked? If the log tab is selected then should only the log button be ticked? What if the user previously selected a script tab and later clicked a log button? should the script button then be unticked? The code would also need to separately store which script tab was last selected, independent from the log tab.

I think it would be simpler and more intuitive for the user to have one view menu button ('Script/Log Window') and one toolbar button (i.e. remove the log button dropdown menu and update the tooltip). Each button would have the save functionality: toggle if the script/log window is visible. The first time the script/log is made visible, it would highlight the first script tab. If the script/log window is then toggled off and later on, then it would just reappear in the same state as when it was last closed (e.g. with the same tab highlighted).

What are your thoughts?
Thanks

@rdstern
Copy link
Collaborator

rdstern commented Mar 19, 2023

@lloyddewit, my suggestions are as follows:
a) In the view menu there should be a single entry. Should we just call it Log/Script, rather than Log/Script Window.? I note we currently call the Output as Output Window, but Data View and Column Metadata and Data Frame Metadata don't say Window. may be Output Window should change to just Output?
b) In the toolbar the main button should return to the last opened tab in the Log/Script window.
c) I would still like to consider a drop down, but rather differently to the current Log and Script options. There are no ticks. Instead it becomes a parallel to the drop-down for the last 10 used dialogues:

Here is the drop-down for the last 10 dialogues:

image

Note, no ticks and rhe names are sort of the names of the dialogues. Now one (big?) use of the scripts correesponds to the idea that there is a To Script button on each dialogue. So using a script is more general, but it will sometimes be like running the R code for a given dialogue - perhaps after tweaking it a bit. So the parallel could be for the log/script pull-down to have the names of the previous log-script tabs in the order they were used? So just like the last used dialogues above.
We could take this perhaps a useful step further, when the To Script is used.

  1. I think we are agreed that the To Script options will add to the last used existing script window. That's if the last used script tab isn't empty. If you would like it to go to an empty tab, then (in the log/script window) make a new tab.
  2. If this is the first time it is used (so there isn't a current script tab), then it makes a new tab and goes there.
  3. I assume that if the log tab was the last one used, then it will also make a new tab.
  4. So I wonder if, when the To script button is used to start a new tab, then it could essentially have the name of the dialogue as the default name?
  5. This default name might need to have underscores, rather than spaces - see the names of some of the dialogues above? Also if the same dialogue is used twice to go to 2 separate tabs, then the name will have to be made unique, e.g. Boxplot1 if Boxplot is already used as a name. (We have this code when reading files, or adding new variables.)
  6. Maybe we also add a 3rd option to the To Script pull-down, namely To New Script. I'd prefer not 2 buttons, because that's starting to get confusing, so just the one addition, which goes to a new tab, gives it the dialogue name, and closes.

image

@lloyddewit
Copy link
Contributor Author

lloyddewit commented Mar 20, 2023

@rdstern Thank you for the analysis and all the great comments.

a) In the view menu there should be a single entry. Should we just call it Log/Script, rather than Log/Script Window.? I note we currently call the Output as Output Window, but Data View and Column Metadata and Data Frame Metadata don't say Window. may be Output Window should change to just Output?
b) In the toolbar the main button should return to the last opened tab in the Log/Script window.

Yes, all agreed

c) I would still like to consider a drop down, but rather differently to the current Log and Script options. There are no ticks. Instead it becomes a parallel to the drop-down for the last 10 used dialogues.

I assume that you mean it should list the last ten tabs opened. For me, this is inconsistent with how other tabbed applications work (e.g. browsers, spreadsheets etc). A browser just opens with all the tabs visible and the last used tab selected. I have never heard a user complaining that they would like to open their browser with a pull down menu that lists the tabs they last viewed. This would be more clicks for the user and the list of tab titles would be less visible and intuitive than just seeing the tabs in the application. In addition, tabs may have been renamed or removed since they were visited. So we would either need to add complexity managing the list, or present the user with an unreliable list.
I personally prefer the simpler and more intuitive behaviour of other tabbed applications that the user will be familiar with. Would you be OK with this?

Now one (big?) use of the scripts corresponds to the idea that there is a To Script button on each dialogue. So ...

If the user clicks the 'To Script' button, then the current behaviour in this PR is to write to the currently open script tab. There are some special cases (e.g. log/script window never opened or not visible; log tab selected and just one script tab, log tab selected and multiple script tabs etc.). I tried to handle all these cases in the code in a way that is simple and intuitive for the user. It is not efficient to describe the special cases here. I suggest that you try the code in this PR and see if is acceptable to merge. If more complexity is needed, then we could add it in a separate PR (could potentially be done by a different developer).
What do you think?

@rdstern
Copy link
Collaborator

rdstern commented Mar 23, 2023

@lloyddewit I am ok with your simpler solution - at least for now. So ignore my comment c) above. I think this means there is no drop down for this item on the toolbar. Nice and simple.

@lloyddewit lloyddewit marked this pull request as ready for review March 29, 2023 10:52
@lloyddewit
Copy link
Contributor Author

@rdstern Please could you test?
Thanks!

Copy link
Collaborator

@rdstern rdstern left a 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 - and I couldn't break it! I did try!
It would be great to merge, so I am approving.

The one thing I couldn't do was rename any of the script tabs? Would that be possible? {perhaps there could be a rename button, and then the current name becomes editable? Or you could double-click on the name and then edit. Or right-click on the name and choose Rename?

Or I have missed the way to rename now?

The help is working, and a message to myself is that we need now to change the help file so it applies to both, i.e. Log/Script Window - and then write the contents.

@lloyddewit
Copy link
Contributor Author

@rdstern Thank you for approving!
The tab naming has not been changed by this PR, it was designed/implemented to be the same as RStudio. The log tab behaves the same as the script tabs behave.
If you save the tab to a file then the tab name is the name of the file.
I hope that's still OK.

@ChrisMarsh82 Please could you do a code review?
(normally I'd ask @N-thony but he's unavailable)

Thanks

Copy link
Contributor

@ChrisMarsh82 ChrisMarsh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good. There is a possible slight tweak that the 'help' button could go to Log Window in the help button if that tab is selected. However, this is definitely not essential

@lloyddewit lloyddewit merged commit a5162b2 into IDEMSInternational:master Mar 31, 2023
2 checks passed
@lloyddewit
Copy link
Contributor Author

This all looks good. There is a possible slight tweak that the 'help' button could go to Log Window in the help button if that tab is selected. However, this is definitely not essential

@rdstern Should we add this to issue #7989 ? Or do you think it's OK to have a combined help page for the whole script window?

@rdstern
Copy link
Collaborator

rdstern commented Mar 31, 2023

@lloyddewit I am happy you merged. I was assuming I would change the help to correspond the the new log/script option. But that's interesting too. Let me ponder more - and perhaps check with @rachelkg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the log window the first tab in the script window
3 participants