FUSETOOLS2-725 - Removing single window limit on didact #365

bfitzpat · 2021-01-05T19:44:46Z

Removing single-window limitation on opening Didact files. https://issues.redhat.com/browse/FUSETOOLS2-725

Exploring issues with caching on a per-window basis (see https://issues.redhat.com/browse/FUSETOOLS2-930)

Signed-off-by: Brian Fitzpatrick bfitzpat@redhat.com

bfitzpat · 2021-01-05T19:49:18Z

Essentially now when the workspace shuts down we cache the details from the first Didact window closed in a given session. In this workspace's case, it opened with the Didact demo, so that's tucked away in the cache. So regardless of what you have open when the workspace closes, you get...

I think maybe we can use the vscode setState/getState API to handle things differently on a per-window basis rather than having a single cache for all windows: https://code.visualstudio.com/api/extension-guides/webview#getstate-and-setstate

bfitzpat · 2021-01-05T19:49:44Z

Definitely a work in progress, but wanted to stash the work.

bfitzpat · 2021-01-08T21:20:28Z

Improvement... now have it working and saving html state on a per-window basis.

Removed all the history functionality, which is no longer necessary.
Updated tests
Working on a small issue with the didact window title being reset for the first window -- only affects the first window though

src/didactWebView.ts

bfitzpat · 2021-01-26T20:27:59Z

Success!!

bfitzpat · 2021-01-26T20:32:39Z

This is a large commit, but the core of the work is in DidactManager and DidactPanel.

apupier

I have not found tests for the new functionality
every time I open a new VS Code isntance, another Didact tutorial page is opened
The welcome page stays blank (other tutorial are loading)
tried the "Writing your first Didact tutorial":
- The "Click here to open the new myfirst.didact.md file" is no more working. The erro rmessage seems to say that the dots has been removed: Unable to open 'didactmdfile.json': Unable to read file '/home/apupier/ws/test-didact/didactmdfile.json' (Error: Unable to resolve non-existing file '/home/apupier/ws/test-didact/didactmdfile.json').
- it simplifies a lot the workflow!
When hitting Ctrl+Shift+V to preview a tutorial that i'm currently writing, it is opening a new tab each time. I woudl expect that for the same file, it is only refreshing the existing one (but sounds possible to have it as an improvement for another iteration)

media/main.js

src/didactManager.ts

src/didactPanel.ts

src/didactPanelSerializer.ts

apupier · 2021-01-27T08:08:35Z

src/didactPanelSerializer.ts

+		this._context = context;
+	}
+
+	public async deserializeWebviewPanel(webviewPanel: WebviewPanel, state: any) : Promise<void> {


I think that it will be cleaner to provide a specific type for the state.
Something like:

class DidactPanelState { public oldBody:string; public oldTitle: string; constructor(oldBody:string, oldTitle: string) { this.oldBody =oldBody; this.oldTitle=oldTitle; } }

Not sure it's needed. There are TWO items in this state, so it's very very minimal. If it was more complex, I might agree.

deactivate the eslint rule which is forbiding usage of any then

@apupier I thought you were against deactivating eslint rules?

Added the necessary rule disablement

I prefer not deactivating them without good reasons but when the rules are not followed, it is then better to deactivate them. it avoids to have noise of rule that you choose to not follow.
It is better to introduce a type instead of disabling the rule. But you do not want. So it remains better to to disable the rule if it is not followed.
By not creatign the type, the type-safe part of TypeScript language is not leveraged. We will hit all the troubles of non-typed languages.

src/extensionFunctions.ts

bfitzpat · 2021-01-27T14:16:04Z

I have not found tests for the new functionality

There really isn't new functionality. All we did was remove the limit on a single window. Not quite sure what kind of tests you expect for that.

every time I open a new VS Code isntance, another Didact tutorial page is opened

Can you clarify what's going on here? In my local testing, it doesn't open additional pages. Is this with the default setting in place to open the default tutorial set in settings?

The welcome page stays blank (other tutorial are loading)

Can you clarify what this means? Maybe provide a video/gif?

The "Click here to open the new myfirst.didact.md file" is no more working. The erro rmessage seems to say that the dots has been removed: Unable to open 'didactmdfile.json': Unable to read file '/home/apupier/ws/test-didact/didactmdfile.json' (Error: Unable to resolve non-existing file '/home/apupier/ws/test-didact/didactmdfile.json').

Will investigate.

it simplifies a lot the workflow!

Great!

When hitting Ctrl+Shift+V to preview a tutorial that i'm currently writing, it is opening a new tab each time. I woudl expect that for the same file, it is only refreshing the existing one (but sounds possible to have it as an improvement for another iteration)

Interesting quirk with this implementation. I will see if there's a way to work around that, perhaps by storing the uri and comparing that when opening a new window so we only open the same one. But let's spin that off into a separate issue I think. Related, but separate.

apupier · 2021-01-27T14:24:04Z

I have not found tests for the new functionality

There really isn't new functionality. All we did was remove the limit on a single window. Not quite sure what kind of tests you expect for that.

that's a big functionality to be able to open 2 didact tutorial at the same time. Also that they keep the state. A test that is openign 2 tutorials and checkign that they both open with correct state woudl very useful.

apupier · 2021-01-27T14:25:18Z

The welcome page stays blank (other tutorial are loading)

Can you clarify what this means? Maybe provide a video/gif?

Open VS code with Didact installed
wait that the welcome page opens
the page is blank

bfitzpat · 2021-01-27T14:25:43Z

that's a big functionality to be able to open 2 didact tutorial at the same time. Also that they keep the state. A test that is openign 2 tutorials and checkign that they both open with correct state woudl very useful.

I don't disagree with you. However, unless you know how to shut down a vscode instance during a test and re-open it, I'm afraid we're kind of out of luck there.

apupier · 2021-01-27T14:39:23Z

that's a big functionality to be able to open 2 didact tutorial at the same time. Also that they keep the state. A test that is openign 2 tutorials and checkign that they both open with correct state woudl very useful.

I don't disagree with you. However, unless you know how to shut down a vscode instance during a test and re-open it, I'm afraid we're kind of out of luck there.

we can have at least teh check that 2 tutorials can be opened side by side.

Doe it really need a shutdown? Can it be a "reload window"? (for whcih VS Code has a command is used when some installation are installed)

bfitzpat · 2021-01-27T16:48:38Z

I believe this latest commit addresses all of the suggestions and issues raised except for the one where if you open a single didact more than once, it opens more than one window so you can have duplicates. Working on that next.

bfitzpat · 2021-01-27T21:15:56Z

Ok, with the last two commits we have a couple of things:

now using the uri effectively to ensure that we don't open more than one window for a given URI
now testing to ensure that the we avoid duplicates
renaming the old webview tests to panel tests to better reflect the refactored work

My question though is this: If I have a Didact tutorial open and re-open it, it overwrites any of the html changes that may have happened since the initial open. For instance, if I checked requirements, those requirements are now green or red with indications of whether the requirements are met. Those html changes are persisted behind the scenes, so if you close the workbench and come back it remembers what was green and red. If you open the tutorial a second time, it just resets the tutorial to its initial html.

Should there be a "Are you sure?" moment in there to ensure that it doesn't overwrite anything they've done without their knowledge? Or is that too much of an intrusion? I mean, we're just talking the state of some html here but still it could be annoying to redo some of those state changes.

bfitzpat · 2021-01-27T21:17:49Z

On the plus side, doing things like writing tutorials and previewing the work is a little easier. Each Ctrl+Alt+V refreshes the window so it automatically updates from the Didact adoc/md to a HTML rendering. You just have to save the file (Ctrl+S) and hit preview again (Ctrl+Alt+V) and it reflects those changes in the same place you dragged the window to.

Not quite as good as a live preview, but close.

apupier · 2021-01-28T09:14:09Z

Should there be a "Are you sure?" moment in there to ensure that it doesn't overwrite anything they've done without their knowledge? Or is that too much of an intrusion? I mean, we're just talking the state of some html here but still it could be annoying to redo some of those state changes.

I think the are you sure step could be nice if their is also a checkbox to disable it fo rnext time. I think this can be handled in another iteration.

apupier · 2021-01-28T09:17:39Z

src/didactManager.ts

+			if (testUri && originalUri) {
+				if (originalUri.toString() === testUri.toString()) {


the two if conditions can be merged

apupier · 2021-01-28T09:20:48Z

src/didactPanelSerializer.ts

+		this._context = context;
+	}
+
+	public async deserializeWebviewPanel(webviewPanel: WebviewPanel, state: any) : Promise<void> {


I prefer not deactivating them without good reasons but when the rules are not followed, it is then better to deactivate them. it avoids to have noise of rule that you choose to not follow.
It is better to introduce a type instead of disabling the rule. But you do not want. So it remains better to to disable the rule if it is not followed.
By not creatign the type, the type-safe part of TypeScript language is not leveraged. We will hit all the troubles of non-typed languages.

bfitzpat · 2021-01-28T15:48:00Z

I prefer not deactivating them without good reasons but when the rules are not followed, it is then better to deactivate them. it avoids to have noise of rule that you choose to not follow.
It is better to introduce a type instead of disabling the rule. But you do not want. So it remains better to to disable the rule if it is not followed.
By not creatign the type, the type-safe part of TypeScript language is not leveraged. We will hit all the troubles of non-typed languages

Though I don't disagree with you in principle, I definitely disagree as a practical matter here. Taking a look around github for other examples (even Microsoft https://github.com/search?q=deserializeWebviewPanel+user%3Amicrosoft+language%3ATypeScript+language%3ATypeScript&type=Code&ref=advsearch&l=TypeScript&l=TypeScript) nobody is making it harder than it needs to be by avoiding the any type. In our case, this typescript file is < 25 lines of code, the purpose is clear, and overcomplicating things to avoid eslinter rules doesn't buy us anything from a practical matter since typescript boils down to javascript in the end.

So the fact that I "do not want" to overcomplicate things is correct. If we truly want to deal with adding a type, that can be done in a follow-on code change, but at this time it seems like a lot more trouble than it's worth.

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

also reuse old panels (based on uri) to avoid opening a new window each time Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

and add test to ensure that if we open the same didact URI twice we only end up with one panel open and not two of them Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

apupier

good feature. The removal of the in-house history is a very good improvement from a maintenance point of view.
The downside of the new code is the non-typed state that is used which is nto optimizing the maintainability.

apupier · 2021-01-29T14:59:26Z

In our case, this typescript file is < 25 lines of code, the purpose is clear

It is softly linked to another file where the setState is used. The day the expected inout is modified, the setStates won't be detected at compile time.

bfitzpat added the work in progress A draft PR or work in progress label Jan 5, 2021

bfitzpat self-assigned this Jan 5, 2021

apupier reviewed Jan 14, 2021

View reviewed changes

src/didactWebView.ts Outdated Show resolved Hide resolved

bfitzpat force-pushed the FUSETOOLS2-725-multiple-didact-windows branch 2 times, most recently from 87e6be3 to a7c1511 Compare January 26, 2021 20:20

bfitzpat added READY FOR REVIEW and removed work in progress A draft PR or work in progress labels Jan 26, 2021

bfitzpat requested review from lhein and apupier January 26, 2021 20:32

apupier suggested changes Jan 27, 2021

View reviewed changes

bfitzpat requested a review from apupier January 27, 2021 21:22

apupier reviewed Jan 28, 2021

View reviewed changes

bfitzpat force-pushed the FUSETOOLS2-725-multiple-didact-windows branch from a1b5e8a to ea70b33 Compare January 28, 2021 15:52

bfitzpat added 2 commits January 29, 2021 06:28

FUSETOOLS2-725 - Removing single window limit on didact

2749b6a

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

updates to support single tutorial window

1d01608

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

bfitzpat added 12 commits January 29, 2021 06:28

fix for title changes

b4bd92a

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

fix issue with disposed webview when saving column

63f1173

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

adding first cut of new didact panel and manager

1c88208

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

first solid pass with most things working

d66c094

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

working through all tests and clean-up

6cbbe75

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

minor cleanup

8dae04b

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

minor tweak after merge

67d9d6e

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

removing commented out code

2c8ee04

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

adding tests and updates after first round of feedback

5225401

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

persist uri for easier identification

56de080

also reuse old panels (based on uri) to avoid opening a new window each time Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

rename old didact webview tests

102f66e

and add test to ensure that if we open the same didact URI twice we only end up with one panel open and not two of them Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

combining if conditions

b29aa41

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>

bfitzpat force-pushed the FUSETOOLS2-725-multiple-didact-windows branch from ea70b33 to b29aa41 Compare January 29, 2021 13:29

bfitzpat requested a review from apupier January 29, 2021 13:30

apupier approved these changes Jan 29, 2021

View reviewed changes

bfitzpat merged commit 98c40e5 into redhat-developer:master Jan 29, 2021

bfitzpat mentioned this pull request Jan 29, 2021

FUSETOOLS2-930 - smarter caching #347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FUSETOOLS2-725 - Removing single window limit on didact #365

FUSETOOLS2-725 - Removing single window limit on didact #365

bfitzpat commented Jan 5, 2021

bfitzpat commented Jan 5, 2021

bfitzpat commented Jan 5, 2021

bfitzpat commented Jan 8, 2021

bfitzpat commented Jan 26, 2021

bfitzpat commented Jan 26, 2021

apupier left a comment

apupier Jan 27, 2021

bfitzpat Jan 27, 2021

apupier Jan 27, 2021

bfitzpat Jan 27, 2021

bfitzpat Jan 27, 2021

apupier Jan 28, 2021 •

edited

Loading

bfitzpat commented Jan 27, 2021

apupier commented Jan 27, 2021

apupier commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

apupier commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

apupier commented Jan 28, 2021

apupier Jan 28, 2021

apupier Jan 28, 2021 •

edited

Loading

bfitzpat commented Jan 28, 2021

apupier left a comment

apupier commented Jan 29, 2021

		if (testUri && originalUri) {
		if (originalUri.toString() === testUri.toString()) {

FUSETOOLS2-725 - Removing single window limit on didact #365

FUSETOOLS2-725 - Removing single window limit on didact #365

Conversation

bfitzpat commented Jan 5, 2021

bfitzpat commented Jan 5, 2021

bfitzpat commented Jan 5, 2021

bfitzpat commented Jan 8, 2021

bfitzpat commented Jan 26, 2021

bfitzpat commented Jan 26, 2021

apupier left a comment

Choose a reason for hiding this comment

apupier Jan 27, 2021

Choose a reason for hiding this comment

bfitzpat Jan 27, 2021

Choose a reason for hiding this comment

apupier Jan 27, 2021

Choose a reason for hiding this comment

bfitzpat Jan 27, 2021

Choose a reason for hiding this comment

bfitzpat Jan 27, 2021

Choose a reason for hiding this comment

apupier Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

bfitzpat commented Jan 27, 2021

apupier commented Jan 27, 2021

apupier commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

apupier commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

bfitzpat commented Jan 27, 2021

apupier commented Jan 28, 2021

apupier Jan 28, 2021

Choose a reason for hiding this comment

apupier Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

bfitzpat commented Jan 28, 2021

apupier left a comment

Choose a reason for hiding this comment

apupier commented Jan 29, 2021

apupier Jan 28, 2021 •

edited

Loading

apupier Jan 28, 2021 •

edited

Loading