Upload command always visible on browser

[federicobozzini]

What it does

This PR makes the Upload Files... menu entry always visible on a browser environment. At the moment the menu entry disappears if no folder is selected on the explorer and I think this might quite counter-intuitive for a lot of users as menu entries don't generally disappear if they are unusable but they just get disabled.

How to test

Just start Theia and check the menu entry for uploading files and the File menu. It should now always be visible.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

@federicobozzini I'm not sure this change works as you intended, a selection of a directory is needed in order for the command to be added in the first place and is not always visible (I think this behavior is odd):

initially (perform a reset workbench layout)

after a directory selection:

[federicobozzini]

@vince-fugnitto My mistake, it should now be fixed.

This weird behavior comes from the fact that the command handler is transformed by the FileSelection.CommandHandler (based on SelectionCommandHandler) which always check if a selection is currently active. Since not every component based on SelectionCommandHandler might need this behavior I've removed from there.

SelectionCommandHandler is currently used by:

• FileSelection.CommandHandler which I think doesn't need this behavior. At the moment it's only used by the FileSystemCommands.UPLOAD so my assumption is based on that.
• ProblemSelection.CommandHandler which needs this behavior. I moved the logic from SelectionCommandHandler to this.
• BulkEditNodeSelection.CommandHandler which is unused at the moment.

[vince-fugnitto]

This is a breaking change that is problematic. We should not update isVisible for all selection command handlers to fix the minor bug. The isVisible would break downstream applications if used.

[federicobozzini]

I've pushed a different solution, with no breaking changes.

[vince-fugnitto]

I'm trying to think about this one a bit more, I'm not sure the proper or cleanest solution would be to force the selection-command-handler to suit our needs but maybe rather create a new handler, or even omit using the selection-command-handler and instead handle things ourselves with the selectionService.

[federicobozzini]

I agree this issue can be fixed in different ways. My basic understanding is that there is no reason why SelectionCommandHandler should force the visibility of item to always consider a selection though. I don't think it's completely unreasonable to let this be decided for every specific command handler getting registered.

In any case I'm happy to change this PR if you have a better suggestion.

[vince-fugnitto]

By definition and name the SelectionCommandHandler is based on selection so that makes sense. If we do not care about the selection like in this case then the handler shouldn't be used and we can instead handle it ourselves rather than try to fit it to our needs for this specific use-case.

[colin-grant-work]

I would recommend simply rewriting the handler for this item not to be a FileSelection.CommandHandler. Since we have access to the SelectionService locally, it would be possible to achieve similar behavior, with the desired modifications, without modifying the FileSelection.CommandHandler class. Though in principle, I agree that the FileSelection.CommandHandler likely shouldn't impose its own constraints on the handlers passed in - setting its own conditions before it runs the handler - but only handle the logic of extracting the selection.

[federicobozzini]

@vince-fugnitto @colin-grant-work I've change this PR to only change the behavior of the upload button. There is some code repetition but I think that's unfortunately inevitable.

Changes were updated

[federicobozzini]

@colin-grant-work @vince-fugnitto Can this PR be merged?

[vince-fugnitto]

The change looks good to me 👍

• upload files... is always visible
• upload files... is enabled if a folder is selected
• upload files... is disabled if a file is selected
• uploading files works as expected

[colin-grant-work]

@federicobozzini, it looks like this is failing the Eclipse Contributor Agreement check because of the shared commits - would you mind pushing a squashed version?

[msujew]

@federicobozzini Do you mind squashing your commits to fix the ECA check?

[federicobozzini]

I've pushed a squashed version, thanks.