-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add Show-ObjectTree
cmdlet
#179
Conversation
@microsoft-github-policy-service agree I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer. |
Hello @tznind First of all, thank you for your contribution I would like to propose you to consider another name, Best regards |
Good point. I've renamed it I also added a StatusBar that shows original object count and selected leaf Type. And I've made it expand |
I like this idea. |
@tznind This is really neat, are you still working on this? I think you can do it iteratively when thinking about adding more features. |
I am now curious about how we can make ocgv and this work together seemlessly. By default selecting something in ocgv and having show-object launch will cause a bunch of screeen repaint. We really need non-full-screen Terminal.Gui apps... gui-cs/Terminal.Gui#272 |
Yes! I'm excited to get back to this. I agree an iterative approach would be good. I've marked it ready for review. Let me know if there are required features for a first version. Theres a lot that would be nice to add later (e.g. search, handling big arrays smarter).
Yes I'd love to explore this also. Maybe a shortcut or something in ocgv to show a modal with the tree for currently selected object (or all objects)? |
This prevents Ctrl+Right causing infinite loop where a property references a parent.
@tig, @tznind, and @SteveL-MSFT are we thinking this is good to review and potentially merge? I can't quite tell from the discussion above. |
Its a yes from me. In terms of iterative improvement of this feature, I have a PR in Terminal.Gui to support filtering in It is merged to develop and will be in the next library release. Once that is out I can update this feature to have search. But can do that on a subsequent PR. But if you want to wait and review it all at once that's fine too. |
If I can get T.Gui v1.11 out before Andrew gets to this we should include it. I'll try in next 24 hrs but no promises. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to pull down and play with it, but here's my CR.
src/Microsoft.PowerShell.ConsoleGuiTools/ShowObjectTreeCmdletCommand.cs
Outdated
Show resolved
Hide resolved
{ | ||
#region Properties | ||
|
||
private const string DataNotQualifiedForGridView = nameof(DataNotQualifiedForGridView); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this named correctly? Shouldn't it be "...ForShowObject"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be consistent in parameters supported between OCGV and this. I think you should add:
-Title
-Filter
-MinUI
-Focus
(if you want to tackle OCGV: Request: Focus a specific row on opening the grid view #127 (comment))-Search
(OCGV: Request: Focus a specific row on opening the grid view #127 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added in support for
-Title
-Filter
-MinUI
I have made the filter logic and ux match console grid view (i.e. Regex support with error label).
I think for -Focus
and -Search
that if it is not in the base ocgv yet then its not much point implementing it in the tree view at this stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usability issues:
I wanted to use How would I do that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add docs. See:
src\Microsoft.PowerShell.ConsoleGuiTools\OutConsoleGridviewCmdletCommand.cs
And to the README.md
{ | ||
#region Properties | ||
|
||
private const string DataNotQualifiedForGridView = nameof(DataNotQualifiedForGridView); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be consistent in parameters supported between OCGV and this. I think you should add:
-Title
-Filter
-MinUI
-Focus
(if you want to tackle OCGV: Request: Focus a specific row on opening the grid view #127 (comment))-Search
(OCGV: Request: Focus a specific row on opening the grid view #127 (comment))
src/Microsoft.PowerShell.ConsoleGuiTools/Microsoft.PowerShell.ConsoleGuiTools.psd1
Show resolved
Hide resolved
Now correctly refers to ObjectTree not GridView
So This gives The current behaviour of We could detect when all the passed objects are IO objects and change the behaviour to enumerate children on expansion instead. Optionally the user could pass a flag to prevent that behaviour? |
@tznind - A bunch of the comments I added above have not been addressed in this PR.
Will you be able to tackle these sometime soon? |
Yes sorry has fallen down my list a bit but can look again at it.
ALthough with the new TreeTable data source in v2 I have been wondering if
just adding a --tree flag to existing command would be easier.
It would allow viewing and selecting child elements in line. For example
pipe some folders to ocgv then expand one and select some files within as
the ogcv command output.
…On Wed, 19 Jul 2023, 15:45 Tig, ***@***.***> wrote:
@tznind <https://github.com/tznind> - A bunch of the comments I added
above have not been addressed in this PR.
- docs
- lowercase show-object isn't working
- inconsistent UI with ocgv
- ...
Will you be able to tackle these sometime soon?
—
Reply to this email directly, view it on GitHub
<#179 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHO3C5FGMXTEYTTZIXFI4VDXQ7XJHANCNFSM6AAAAAATQ2JIDY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
The default 'child getter' is to enumerate the properties on the PSObject and express as I have added a 'special cases' method called I am open to suggestions on how to do this. Some ideas I thought about included:
I don't really like the |
@tig are you happy enough with this for me to do a review and potential merge now? |
@tznind @tig really cool work! Can't wait to figure out how it will integrate with Write-FormatTreeView so that any object can be formatted as a tree! |
@tznind can you add support for |
I have added these settings in 3c654e9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! I'm going to merge it and then run it through an automated formatter because I don't believe in requesting just formatting / whitespace changes. I'm not super familiar with the code but I didn't spot anything obvious. Let's get it in!
PR Summary
Adds the
out-ShowObject
command which presents the passed objects in a tree. Below each object passed appear its public fields/properties.TreeView
fetches children as branches are expanded so the full tree is not built up front which helps with performance in cases of deep or expansive nesting.PR Context
This is my first time working with powershell commandlets so please be gentle.
I thought I would look into implementing #178 . Let me know if this is not what you were expecting. If its not wildly out.
I am happy to look into adding whatever else is required (e.g. filtering / highlighting etc).