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

view containers integration #5665

Merged
merged 38 commits into from
Aug 3, 2019
Merged

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Jul 8, 2019

New container contributed by the plugin system:
Screen Shot 2019-07-09 at 16 53 28

Views contributed to the explorer container by the plugin system:
Screen Shot 2019-07-11 at 16 23 30

What it does

  • close [shell] view containers and tabs #5058 - complete integration of view containers in the plugin system and shell
  • [vscode] support activation events #4199 - support onView activation event
  • register views after all view containers are registered
  • fix support view title menu contributions #4174 - support view/title menu contribution
  • support more view/title menu contribution
  • pre-defined views
  • a command to toggle view container in the view menu and the command palette
  • commands to toggle view containers contributed by plugin system
  • Open View command
  • quick open view prefix
  • implemented TreeItem2.label proposed API for sample extension
    3ea5c9ddbebd8ec68e3b821f9c39c3ec785fde97/src/vs/vscode.proposed.d.ts#L1436-L1477
  • fix Align vscode views contents with header file #4637 - single-view view-container mode
    • view caption should be inlined into the view container caption
    • view toolbar items should be inlined into the view container toolbar
      • scm
      • explorer
      • plugin views
    • toolbar context menu should allow to hide/show view parts
  • show view container part's toolbar only on hover
  • plugin tree views: not enough space between icon and text if the width is small
  • restoration layout of view container on reload
  • view container activation
    • should restore active part on restore of shell layout
    • should restore previous active part on activation, or activate the first expanded or visible part, or receive focus itself
  • shell: handle activation and reveal requests recursively
  • fix UI tests
  • shell layout versioning and backward-compatibility
  • integrate remove one colon from double colons in the code #5718

How to test

What to pay attention:

  • keyboard navigation: one can navigate between view parts with keyboard plus collapse and expand them, try to drag and drop as well
  • activation (focus): a proper part is focused
  • restoration of layout
  • explorer in multi-root workspace

Review checklist

  • as an author, I have thoroughly tested my changes and carefully reviewed following the review guidelines

Reminder for reviewers

@akosyakov akosyakov added shell issues related to the core shell vscode issues related to VSCode compatibility labels Jul 10, 2019
@akosyakov akosyakov force-pushed the ak/view_containers_integration branch 7 times, most recently from 02acfac to 280d9f0 Compare July 15, 2019 21:46
@akosyakov akosyakov force-pushed the ak/view_containers_integration branch from 280d9f0 to b68ba28 Compare July 16, 2019 20:02
@akosyakov
Copy link
Member Author

@tetchel @ariel-bentu could you give it a try with your extensions contributing view containers and views? Feedback is very welcomed!

@tetchel
Copy link
Contributor

tetchel commented Jul 17, 2019

It does not appear to have worked for me.

        "views": {
            "explorer": [
                {
                    "id": "%viewID%",
                    "name": "%viewName%"
                }
            ]
        },

where:

    "viewName": "Codewind",
    "viewID": "ext.cw.explorer",

this is the same snippet that works in VS Code.

VS Code:
image

Theia:
image

Note the Explorer icon broke, and there is still just the one main view.

@akosyakov
Copy link
Member Author

akosyakov commented Jul 17, 2019

@tetchel why do you use internalization for internal constants? it should be used for user visible text. Anyway i will add handling for it, since it appears to happen :)

It would be good if you direct me how i can get the same results, i.e. which extension to try with which project.

@akosyakov
Copy link
Member Author

akosyakov commented Jul 17, 2019

@tetchel Could you also try to do Reset Workbench Layout? Since layout changed so much old layout data probably cannot be loaded.

@svenefftinge @spoenemann @kittaakos I'm thinking to add a
layout version to layout data, so we increase it each time to reject old layout state. It would allow to make backward incompatible changes to the layout without breaking clients.

@tetchel
Copy link
Contributor

tetchel commented Jul 17, 2019

I suppose it's true that there's no point in externalizing the viewID. let me see if that fixes it.

@kittaakos
Copy link
Contributor

kittaakos commented Jul 17, 2019

I'm thinking to add a
layout version to layout data

I had a deja vu after reading your comment, @akosyakov 😊 We already have something for it:
https://github.com/theia-ide/theia/blob/613069fc87169738999adab4044bc0530e54cb74/packages/core/src/browser/shell/application-shell.ts#L48

The related task was: #1075
PR: #1603

@akosyakov
Copy link
Member Author

@kittaakos oh, cool, i can just increase it!

@akosyakov
Copy link
Member Author

akosyakov commented Jul 17, 2019

@tetchel could you pull and try again, I've increased layout data version, so it should be loaded from scratch for you now

@tetchel
Copy link
Contributor

tetchel commented Jul 17, 2019

After changing the viewID and pulling, it worked! 🎉
image

@akosyakov
Copy link
Member Author

@tetchel cool! I've noticed that tree elements look different for you. Could you please file issues for such UI differences as well?

@tetchel
Copy link
Contributor

tetchel commented Jul 18, 2019

Do you mean the differences in text on the Projects and No Projects item? That's by design in my code.
or is there some other difference?

@akosyakov
Copy link
Member Author

@tetchel there is for example Click here to create... in VS Code, but not in Theia

@tetchel
Copy link
Contributor

tetchel commented Jul 18, 2019

Yea that is intentional because in vs code clicking that item runs the Create Project command. But I removed it in theia because on-click tree item commands don't work. Similarly in VS Code Projects (Local) is correct but in my Theia case it is confusing to a user ("local" doesn't really apply) so I removed it.

There are some intentional differences, which is why I opened #5501

Bottom line is, my tree works great with this change.

@akosyakov
Copy link
Member Author

ok, do we have an issue already for But I removed it in theia because on-click tree item commands don't work. @tetchel ?

@tetchel
Copy link
Contributor

tetchel commented Jul 18, 2019

I thought we did, but it was just a few comments on #5661. I just opened #5744, thanks for the bump.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
- labels should be updated when a workspace changed
- labels should include a workspace only if it is a multi-root

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
- it used to leak a handler

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
otherwise collapsed view containers are not restored

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
widgets should handle key up and down to change scrolling programmatically

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov force-pushed the ak/view_containers_integration branch from 94c8539 to 80c4e86 Compare August 2, 2019 14:29
@akosyakov
Copy link
Member Author

@AlexTugarev triy again, I've messed up rebase last time

@AlexTugarev
Copy link
Contributor

Verified, the most recent change does solve the issue with moved views. Cool!

@akosyakov
Copy link
Member Author

akosyakov commented Aug 2, 2019

I'm merging if noone objects when the build is green. Please open new issues if something breaks afterwards. It will be my top-priority to fix them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell issues related to the core shell vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants