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

[master] adding 'writable' checks to some of the menu and toolbar options #5376

Merged
merged 6 commits into from Sep 28, 2018

Conversation

@dharmaquark
Copy link
Contributor

@dharmaquark dharmaquark commented Sep 24, 2018

adding logic to check 'writable' for the following menu items:

  • Save
  • Save As ...
  • Save All .. (only disabling if ALL files are not writable)
  • Rename

toolbar items:

  • Save

as a followup to this PR, when the refactoring that @jasongrout is working on around 'Save Notebook with Extras' lands, I'm hoping to add a similar 'writable' check (tricky to do that now without adding cyclic dependencies between the docmanager and mainmenu extensions)

ivanov
Copy link
Member

ivanov commented on 11e1c83 Sep 19, 2018

Choose a reason for hiding this comment

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

Does it make sense to just shortcut and skip checking all the other widgets and return true; here?

ivanov
Copy link
Member

ivanov commented on 11e1c83 Sep 19, 2018

Choose a reason for hiding this comment

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

Then we can keep the return false; here and not need the anyWriteable variable.

jasongrout
Copy link
Contributor

jasongrout commented on 11e1c83 Sep 24, 2018

Choose a reason for hiding this comment

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

Perhaps we could set the context variable first, then check to see if context is writable, so we don't have two calls to contextForWidget:

let context = docManager.contextForWidget(widget)
if (context && context.contentsModel && context.contentsModel.writable) { ... }

ivanov
Copy link
Member

ivanov commented on 11e1c83 Sep 19, 2018

Choose a reason for hiding this comment

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

Does it make sense to just shortcut and skip checking all the other widgets and return true; here?

ivanov
Copy link
Member

ivanov commented on 11e1c83 Sep 19, 2018

Choose a reason for hiding this comment

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

Then we can keep the return false; here and not need the anyWriteable variable.

jasongrout
Copy link
Contributor

jasongrout commented on 11e1c83 Sep 24, 2018

Choose a reason for hiding this comment

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

Perhaps we could set the context variable first, then check to see if context is writable, so we don't have two calls to contextForWidget:

let context = docManager.contextForWidget(widget)
if (context && context.contentsModel && context.contentsModel.writable) { ... }

Copy link
Contributor

@jasongrout jasongrout left a comment

Looks great to me! I noted one place where I think the code structure could be a bit easier to follow.

if (currentWidget) {
const context = docManager.contextForWidget(currentWidget);
return !!(
currentWidget &&
Copy link
Contributor

@jasongrout jasongrout Sep 25, 2018

Choose a reason for hiding this comment

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

We don't need to check for currentWidget any more since we're inside the if block. On the other hand, we could also make the currentWidget check an early exit, which may be easier to follow:

if (!currentWidget) { return false; }
const context = ...
return !!(...)

@jasongrout jasongrout added this to the 0.35 milestone Sep 25, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 25, 2018

CC @saulshanabrook, who wrote the save with extras command.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 25, 2018

@saulshanabrook saulshanabrook self-requested a review Sep 25, 2018
Copy link
Member

@saulshanabrook saulshanabrook left a comment

Looks good to me! Thank you for this. Will this close #3917 ?

If you would like to add similar logic to the Save Notebook with View State command it is defined here

How would it look to add writable as a property to the context so that you don't have to repeat the context && context.contentsModel && context.contentModel.writable twice? (EDIT: This may or may not make sense, just an idea)

@@ -85,7 +85,8 @@ export namespace ToolbarItems {
}
});
},
tooltip: 'Save the notebook contents and create checkpoint'
tooltip: 'Save the notebook contents and create checkpoint',
enabled: panel.context.contentsModel.writable
Copy link
Member

@saulshanabrook saulshanabrook Sep 25, 2018

Choose a reason for hiding this comment

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

Looking at test failure of TypeError: Cannot read property 'writable' of null maybe it is coming from here? Since there isn't all the && checks?

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Sep 26, 2018

@dharmaquark I am gonna clone this and see if I can get "Save Notebook with View State" to respect the readonly state as well.

@saulshanabrook saulshanabrook self-assigned this Sep 26, 2018
@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 26, 2018

Ok thank you! I can go ahead and address the other feedback from Jason and fix the broken test on my end, if that works!

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Sep 26, 2018

@dharmaquark Sounds good! Thanks!

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Sep 27, 2018

I added a change in #5391 to make the persist and save command reflect the save commands status.

Either that should be merged first and then this rebased/merged from master or you could merge that branch into your branch. It shouldn't require any other changes in this PR to get it to work.

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 27, 2018

Ok thanks!! Apologies for the lag in reply/PR updates (on the road this week), but I’m aiming to get better WiFi access/the PR updates in by EOD (review edits, fixing tests), happy to rebase/merge in your changes if they land first!

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Sep 27, 2018

No worries! Thanks so much for tackling this :)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

I think there are no conflicts with #5391, and I went ahead and merged that into master, so you don't need to worry about merging that into this PR. Here are some other points that have come up in discussion over the past few days:

  1. Save As gives the full path to the file (and you can change the path), and doesn't depend on the original file being writable, so Save As probably should never be grayed (unless you want to check to see if all directories in the entire filesystem are readonly).
  2. A file being readonly doesn't prevent it from being renamed. Renaming a file is only prevented if the directory is readonly. It does look like we don't allow a file to change directories when renaming, so it looks like it sufficient to just check to see if the directory is readonly.

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 28, 2018

Ok makes sense! I’ll take out the writable check for ‘save as’ and update the rename logic to check the ‘readOnly’ property (is ‘context.model.readOnly’ a directory property in jupyterlab, versus file property? Or is there something else that I could access from docmanager-extension’s index.ts to check the readability of the directory?) thanks!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

I think directories have a readOnly property too.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 28, 2018

Files and directories have a writable attribute in their model:

readonly writable: boolean;

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

writable

Ah, right: "writable", not "readOnly"

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 28, 2018

fixed the null checks in default-toolbar.ts to see if this fixes the test failures, made @jasongrout 's suggested change to the isWritable function (made the currentWidget check an early exit)
(still TODO: I'm working now on the change to check directory 'writable' now for the 'rename' menu item)

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 28, 2018

for accessing the contentsModel of the current directory, is there an equivalent to the file's docManager.contextForWidget(widget) that I can use to get the directory's context/contentsModel from docmanager-extension? Or I'm wondering if there is there another way, through the shell (some equivalent to shell.currentWidget?), or through services.contents to check the directory's writable property? Looking through the other packages now to see if I can find similar logic

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

is there an equivalent to the file's docManager.contextForWidget(widget) that I can use to get the directory's context/contentsModel from docmanager-extension?

I'm looking too, and so far haven't found anything. I think this may require some extra work - perhaps we can make the rename enabling a separate issue, and put it back to just using isEnabled for now?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

perhaps we can make the rename enabling a separate issue

We probably also want to cover the renaming logic in the filebrowser as well to check the directory permissions.

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 28, 2018

is there an equivalent to the file's docManager.contextForWidget(widget) that I can use to get the directory's context/contentsModel from docmanager-extension?

I'm looking too, and so far haven't found anything. I think this may require some extra work - perhaps we can make the rename enabling a separate issue, and put it back to just using isEnabled for now?

makes sense, would rather take the time to have the check done the right way! I took out the rename for now, makes sense to handle that in a separate, later PR since it will involve more changes!

@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 28, 2018

looks like the tests are working again- let me know if there's anything else I missed addressing from the review comments that I'll need to change before this can get merged! Many thanks for reviewing!!

Copy link
Member

@saulshanabrook saulshanabrook left a comment

Looks good to me! Thank you for working through the changes.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2018

I just checked it out one more time, and it works and looks great! Thanks again!

@jasongrout jasongrout merged commit 4d9ba2f into jupyterlab:master Sep 28, 2018
2 checks passed
@dharmaquark
Copy link
Contributor Author

@dharmaquark dharmaquark commented Sep 28, 2018

awesome! thanks for all the help in getting this one merged in!

@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants