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

Consider refactoring CommandPalette + Switcher => LiveFilteringListView #7285

Open
DHowett opened this issue Aug 13, 2020 · 11 comments
Open
Labels
Area-CmdPal Command Palette issues and features Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Milestone

Comments

@DHowett
Copy link
Member

DHowett commented Aug 13, 2020

There's a good core of an implementation here for a live filtering list view, and I believe that having it serve two leaders (command palette and tab switcher) may unnecessarily complicate its implementation.

Is there something we can do about that?

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 13, 2020
@DHowett DHowett added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 13, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 13, 2020
@DHowett DHowett added this to the Terminal Backlog milestone Aug 13, 2020
@DHowett DHowett added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 13, 2020
@Don-Vito
Copy link
Contributor

@DHowett - I guess I am causing a lot of headache for you with tons of small PRs (realized how distracting this can be after reading https://www.wired.com/story/open-source-coders-few-tired/).

I cannot work on big tasks, especially the ones that are prioritized (very hard to combine it with a family and 12h/day work), especially the ones that require a deep understanding of the architecture and the product. Hence I am hunting for quick wins (up to one night of coding) that are close to surface (I have no clue how rendering works, why VT, etc., etc.)

So probably instead I can work on a stuff like this

  • it will take me a while
  • the interface is simple
  • it is quite standalone (a lot of coding that can be merged, and then a short integration)
  • it is not on the roadmap

Actually I would like to do even more with the Command Palette, which is IMHO much more complex than it should be. This is at least how I would like to see it:

  • We need a simple ephemeral host control with no business logic (it should open nicely and close on time - something like a flyout)
  • Inside the host we will have
  • a control for Tab Switching (with the tab search capabilities)
  • a control for Actions (with nested commands capabilities)
  • a control for Command Lines
  • a logic that switches between their visibility (can be fully Xaml)
  • Terminal page will work with these controls directly (updating data via model-viewmodel binding, and registering to business domain events like "CommandSelectedForExecution", "TabSelectedForPreview", "TabSelectedForSwitch")
  • LiveFilteringListView will implement a significant part of the common logic (filtered command has already extracted the filtering and sorting, but we still need to extract the list rendering and the navigation)

But this one I am afraid to work on, as it will take me few months to implement and even months to merge - I will probably die while merging conflicts.

Long story short - let me know how I can be of a greater use (and a lesser pita). What would you prefer: polishes like today, stuff like the above or something else (given the limitations I mentioned)?

@zadjii-msft - FYI.

@zadjii-msft
Copy link
Member

I know you originally asked Dustin, but these are my thoughts on the matter:

Absolutely don't worry about sending out a ton of small PRs. I kinda like it, they're more easily digestible and reviewable IMO. And at this rate, you've probably committed more code to the Command Palette than I have 😆 It's been nice to have someone working on UX improvements while I've been so heads-down on architectural pieces.

This does seem like a task that's relatively right up your alley, but if it's not something that works with your own contribution schedule, don't worry about it. I think the team is happy to work on more of the big-picture architectural work and let the community come in with the smaller polish fixes and features. I think at the end of the day, you should work on whatever you're passionate about and and think you can ship. I have no doubt that you'd be able to get this one done, but if you want to work on 10 other smaller fixes instead, that's great too.

That being said, I don't think I'd worry too much about merge conflicts till the end of the year - probably a lot less code flow than usual with all the holidays coming up 😄

@Don-Vito
Copy link
Contributor

Don-Vito commented Nov 23, 2020

I know you originally asked Dustin, but these are my thoughts on the matter:

@zadjii-msft - I really value your opinion as well, as you are mostly the one working with me! 💟

I asked Dustin while assuming he suffers most: usually it's an area expert + Dustin who review the code (in addition to all other context switches he has being a lead).

@DHowett
Copy link
Member Author

DHowett commented Nov 24, 2020

@Don-Vito i love smaller PRs. They're easy to cherry-pick and when you look back at the history there's a bunch of individual pure changes -- easily revertable, easily blameable, etc.

I'm always asking folks on the team to please break this up into multiple PRs. Sometimes it works ;) and sometimes it doesn't!

@DHowett
Copy link
Member Author

DHowett commented Nov 24, 2020

That having been said: this is a pretty intense refactoring, so I would totally not expect you to (if you choose to do it) break it into smaller chunks. Sometimes you need to rip the band-aid off and come in and wrecking-ball something. miniksa has done that more than once 😁

@Don-Vito
Copy link
Contributor

I absolutely agree that PRs must be small (I am not smart enough to review large ones, so once I was coding it kept me and the team disciplined). So even a larger contribution will arrive in small chunks (probably some history rewriting will be required to achieve this 😊)

The question was what will work better - many small contributions (some bug bashing and quick wins) or a more rare but significant contributions (still split into numerous PRs).

Right now I am happy with the sole fact of writing code so I will probably enjoy any kind of tasks.

@DHowett
Copy link
Member Author

DHowett commented Nov 24, 2020

Right now I am happy with the sole fact of writing code so I will probably enjoy any kind of tasks.

That works for us. Either size is fine -- I'm not gonna be overburdened by a helpful, responsive and well-meaning community member at any rate. 😁

@Don-Vito
Copy link
Contributor

OK.. hopefully I have a plan how to achieve this gradually. Since I cannot edit this one - creating a new ticket.

@DHowett
Copy link
Member Author

DHowett commented Nov 27, 2020

@Don-Vito you can leave your design plan as a comment here and I can update it as necessary

In general our issues are very fluid. 😄

@Don-Vito
Copy link
Contributor

@Don-Vito you can leave your design plan as a comment here and I can update it as necessary

In general our issues are very fluid. 😄

I created a separate one, because I am not sure it will fully cover this one, please see #8415

@DHowett
Copy link
Member Author

DHowett commented Nov 27, 2020

(If i file an issue titled “make x do y”, the title is usually interpretable as “i am uncomfortable with x” and “maybe y would be better?” But as a dev lead I should work on not imposing my opinion quite so much!)

@zadjii-msft zadjii-msft added the Area-CmdPal Command Palette issues and features label Dec 1, 2020
@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CmdPal Command Palette issues and features Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

3 participants