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

feat: display settings in preferences #13

Closed

Conversation

yurii-lunha-stansassets

Purpose of this PR

Implemented feature #11

Testing status

  • No tests have been added.

Manual testing status

Tested manually in using for the Package Manager plugin

Comments to reviewers

1. Added the About page in user preferences (screenshot), which will display "about us" info and at the same time I referenced its code as a simple example of implementing preferences window. What do you think about it?

2. Implemented public TabController that can be useful for developing other plugins, and StansAssets.Plugins.Editor.PackageSettingsWindow<> can use it too (instead of ButtonStrip+ScrollView). It would be better to add it in another PR but PackagePreferencesWindow is dependent on it.

About page
image

Reference example
image

Copy link
Contributor

@kostiantyn-koretskyi kostiantyn-koretskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few changes required

Comment on lines 38 to 49
/// <summary>
/// Use to define custom tabbed menu
/// </summary>
/// <param name="tabsButtons"><see cref="ButtonStrip"/> buttons used to switch between tabs</param>
/// <param name="tabsContainer"><see cref="ScrollView"/> the container that will display the contents of the tabs</param>
public TabController(ButtonStrip tabsButtons, ScrollView tabsContainer)
{
m_TabsButtons = tabsButtons;
m_TabsContainer = tabsContainer;

Init();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid keeping unused constructor(all unused code). When it will be necessary it will be added as separated task

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Comment on lines 119 to 122
if (!m_Tabs.Any())
{
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of code can be used before foreach (var tab in m_Tabs) to avoid additional foreach state check

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved

SettingsScope.User,
new[] { "stan", "plugin", "dev", "kit" })
{
label = "About"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it must be - Plugin Dev Kit

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restructured

}

var packageInfo = GetPackageInfo();
rootElement.Q<Label>("display-name").text = packageInfo.displayName.Remove(0, "Stans Assets - ".Length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typos in Stans assets -> Stan's Assets

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restructured

Copy link

@pavlo-klymentenko-legacy pavlo-klymentenko-legacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! But still needs some love.
Added the video + script below with all the details.

bandicam.2023-05-10.12-54-35-613.mp4
  1. Switching between Group and page
  2. Changes doesnt represent when 2 winows are open
  3. TabController mimics base PackageWindow
  4. Registration process (attribute)? Activator.CreateInstance?
    3.1 GetPackageInfo should reused for registration (keywords, name etc)
  5. PackagePreferencesWindow.cs line 31
  6. CONTRIBUTING.md meta spams (foundation package update from 1.0.16 to 1.0.27)

@yurii-lunha-stansassets
Copy link
Author

Fixed - Switching between Group and page
Not resolved- Changes doesn't represent when 2 windows are open
Refactored - TabController mimics base PackageWindow
Refactored- Registration process (attribute)? Activator.CreateInstance?
Refactored- GetPackageInfo should reuse for registration (keywords, name, etc)
Refactored- PackagePreferencesWindow.cs line 31
Updated - CONTRIBUTING.md meta spams (foundation package update from 1.0.16 to 1.0.27)

Please take a look at #12 - I need these changes for other features too

@pavlo-klymentenko
Copy link
Contributor

All the changes merged with newer PR, that's why this PR is going to be closed.
All credits to @yurii-lunha-stansassets for updates!

@pavlo-klymentenko pavlo-klymentenko deleted the feat/display-settings-in-preferences branch August 15, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Display plugin settings in preferences/project preferences
4 participants