Vscode-extension-for-zowe: USS explorer - Download optimisation

[stepanzharychevbroadcom]

Currently we allow to download any file despite of its size. Even if it's 100Mb we still will initiate the download not even asking user for permission which is definitely not correct. Following use cases are for improving of such experience.

Warning for the file which is bigger than 2 Mb:
1) User tries to open/pull file;
2) User sees message which warns him about size of this file (Message: "File is bigger than 2Mb. Are you sure that you want to proceed with download? File will be opened automatically once download is finished");
3) Interaction: (Theia requires special attention here)

• If User clicks "Yes" - initiate downloading of the file;

- If User clicks "No" - do nothing;

Handling for _downloading_ state of the file:
1) User initiates download in any way;
2) User sees that icon of corresponding node has changed to reflect the status;
3) Once file is downloaded icon changes to _downloaded_ status;

Those changes are valid for both binary and text files.

[stepanzharychevbroadcom]

For design checking I attach source of downloading icon here:
document_downloading.svg.zip

[jalel01]

@jelaplan what are your thoughts here?

There are some concerns around the download icon only being applicable for USS and not for Datasets which use a loading indicator instead.

[jelaplan]

@jalel01 I'm not sure I follow. I would tend to agree that we should be consistent on Data sets and on USS files.

What is the loading indicator that you mention with regard to Data sets? It seems that Stepan is trying to introduce a blue animated file icon for USS files to indicate the file is downloading. I guess that is most valuable for large files that take a while to download. I tried downloading a Dataset and I get a message in the lower right that reads "opening data set." That is some feedback but it isn't animated so if the file takes 30 seconds or 2 minutes to download, it becomes stale. I like the idea of some sort of status animating but honestly, I'm not a fan of the blue animated icon. I'd prefer to have @bubpa01 create something.

I've also seen situations where a blurred blue line moves back and forth across the header sort of Knight rider esque to indicate the system is working. I also wondered about animating the cloud icon. Did we see an example of that in the past?

Here is the Knight Rider thing.

[jelaplan]

@jalel01 I reviewed this with Stepan. I basically agree that a animated file icon is more informative than the general purpose status animation. But, I don't put a great deal of value on doing this because there are more important things to work on. The status indicator we have is good enough and we haven't done any testing or gotten customer input that there is an issue. As the PO, I'll leave it to you to decide on prioritization of tasks.

[stepanzharychevbroadcom]

Added PR: https://github.com/zowe/vscode-extension-for-zowe/pull/587
New icons will be added later.

[Colin-Stone]

The generic approach since v1.2 for all lists and other long running tasks was to use Progress indicator wrapper around all longer running functions. The original rationale was 'Knight Rider' was easily missed.

I'm fine with doubling up with an animated icon in certain circumstances but don't think we want a mixture of some with the progress indicator and some with the animated icon. Users will be used to looking to the bottom right status message

[Colin-Stone]

[stepanzharychevbroadcom]

@Colin-Stone The purpose of this change with downloading icon was to locate which files are currently processed, so user will understand it. Current message with "Opening USS file..." is definitely useful but doesn't give you desired level of details.

[stepanzharychevbroadcom]

@jelaplan According to your recommendation I added another implementation to the downloading icon.

[stepanzharychevbroadcom]

@Colin-Stone @zFernand0 @jellypuno All the new code proposals are described here:
Issue #472 - improvements.pdf

[jelaplan]

@stepanzharychevbroadcom Looks great.

[VitGottwald]

@stepanzharychevbroadcom I strongly disagree with the introduction of global mutable state to avoid "passing arguments". Global mutable state introduces coupling between components that makes refactoring nearly impossible down the the road.
Not only that, it is the source of unpredictability because anyone can change it. It makes tracking down issues time consuming at the very least. Any comments @Colin-Stone @zFernand0?

[Colin-Stone]

I don't understand the pros and cons of the mutable state so have to defer to others who have that deeper understanding.
I do understand the idea of having a structure as originally proposed by @zFernand0 along the lines of IThisMethodOptions which allows callers to pass the elements they want and exclude ones they don't.
Maybe simpler is better as it will be far more obvious to everyone.

[VitGottwald]

@Colin-Stone do you have a link to the proposal by @zFernand0 ?

[Colin-Stone]

No it was just literally along the lines of for example
fileFunction(session: Session, IZoweUSSOptions {
inputFilePath?:string,
ussFilePath?:string,
binary?: boolean,
localEncoding?: string,
task?: any,
etag?: string,
returnEtag?: string
});

[stepanzharychevbroadcom]

@Colin-Stone @zFernand0 @VitGottwald Just to add few common cases which we should resolve somehow (with cache or without):

1. We have structure which sits inside of USSSessionNode: binaryFiles and stores mapping for files which are marked as binary, to my mind it's already a mutable state which should be changed somehow then;
2. Keeping state of the tree between search query changes (so downloaded, downloading, binary and etc. statuses won't be flushed);
3. Ability to affect specific flows even if they're separated by the execution time (something like what is already introduced in Cache service), so if one flow requests specific change inside another one we can store it somewhere;

And as I mentioned before, I'm not a very big fun of changing interfaces of 3-4 methods, when in fact I just need to deliver a flag from A to B.

[VitGottwald]

@stepanzharychevbroadcom changing interfaces and passing arguments is not a good reason enough to introduce state. I understand it is "easy". But it is definitely not "simple" as it comes with big ramifications. see https://www.infoq.com/presentations/Simple-Made-Easy/ .

Introducing state should be the last resort we go to after we have exhausted all other options.

[stepanzharychevbroadcom]

@VitGottwald Then, please, propose other options, because from the perspective of my view we need to somehow resolve the issues which are listed above. Cache to my mind is just an instrument to resolve specific spectre of tasks, if we use it wisely we should be good.

[jellypuno]

I do understand the idea of having a structure as originally proposed by @zFernand0 along the lines of IThisMethodOptions which allows callers to pass the elements they want and exclude ones they don't.
Maybe simpler is better as it will be far more obvious to everyone.

@Colin-Stone suggested something. Maybe it's good to consider this. I do understand this approach as well #justmy2cents

[VitGottwald]

@stepanzharychevbroadcom I will definitely take a closer look.

First of all we are mixing two concerns here.

1. The need to remember user decisions not to ask them all over again
2. Code structure - the fact that we pass an argument through multiple functions

we should separate them and discuss them independently

[jellypuno]

I agree with this. I suggest to remove the code structure and add it as a new issue then we can discuss this as a team.

[VitGottwald]

For 1 we should strive to have the absolute minimum of state needed - ideally zero. But it seems that at some point we will be forced to have some. We have to do a research what options are available to us to persist state and have a thorough discussion before adopting one.
For the purpose of this ticket I recommend dropping the cache completely and having one extra prompt to the user.

[VitGottwald]

1. Is a matter of refactoring. I leave it to @stepanzharychevbroadcom how to approach it. It may be better to put it to a separate ticket and only address the size limitation in this issue / PR.

[stepanzharychevbroadcom]

@VitGottwald @jellypuno I can move it, but keep in mind that in this case we will have broken UX flow and users will see limitation message over and over again. I would suggest to discuss on closest meetings and decide what to do all together.

[Colin-Stone]

One final point regarding state. The original developer who added the flip switch between binary and text simply added an entry to settings to indicate that the user had manually selected the binary choice. This may have been a little clunky but it worked fine, persisted and was easily understandable. I may have missed this being removed at some point

[stepanzharychevbroadcom]

I moved essential things related to https://github.com/zowe/vscode-extension-for-zowe/issues/593 out from #472 PR and closed it before we figure out about the priorities there. Logic with download file size check may be the concern, but it requires caching to work properly, so we need to discuss it first as well.

[zFernand0]

I agree with this. I suggest to remove the code structure and add it as a new issue then we can discuss this as a team.

I know it's probably just me who can't seem to find it :cry:, but do we have an issue for the specific refactoring discussion related to saving the state of the extension?