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

EAGLE-1310: Allow users to change node's category, even when the built-in palettes are not loaded #783

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

james-strauss-uwa
Copy link
Collaborator

@james-strauss-uwa james-strauss-uwa commented Nov 21, 2024

When the built-in palettes are not loaded. Either because the user has turned off the "Open Default Palette on Startup" setting, or because the user is offline. Previously, a list of categories to change to, could not be built.

Now as a fallback, we build the list of categories from the data in CategoryData.cData

We're unable to add/remove fields to match the template in the built-in palette, but at least it allows users' to change categories.

To test:

  • with built-in palettes loaded
  • create a File node
  • change the category of the node to "S3" using the drop-down box in the graph inspector
  • observe that a number of fields are added to the node
  • turn off the "Open Default Palette on Startup" setting
  • reload EAGLE
  • observe that the built-in palettes were not loaded
  • create a File node
  • change category to "S3"
  • observe that the category does change, but that no fields are added to the node

Summary by Sourcery

Enable users to change a node's category without relying on built-in palettes by using CategoryData.cData as a fallback. Enhance the logic for determining eligible node categories when no node is selected.

New Features:

  • Allow users to change a node's category even when the built-in palettes are not loaded by using data from CategoryData.cData as a fallback.

Enhancements:

  • Improve the logic for determining eligible node categories by handling cases where the selected node is null.

@james-strauss-uwa james-strauss-uwa self-assigned this Nov 21, 2024
Copy link
Contributor

sourcery-ai bot commented Nov 21, 2024

Reviewer's Guide by Sourcery

This PR modifies the node category change functionality to work even when the built-in palettes are not loaded. The implementation now includes a fallback mechanism that uses CategoryData.cData when the built-in palette is unavailable, and the category change logic has been restructured to handle cases where template matching isn't possible.

Sequence diagram for changing node category without built-in palettes

sequenceDiagram
    actor User
    participant Eagle
    participant Utils
    participant Palette
    participant Node

    User->>Eagle: Request to change node category
    Eagle->>Palette: Find built-in palette
    alt Palette not found
        Eagle->>Utils: Show notification about missing palette
        Eagle->>Utils: Get categories from CategoryData.cData
    else Palette found
        Eagle->>Palette: Find node prototypes for old and new categories
        alt Prototypes not found
            Eagle->>Utils: Log warning about missing prototypes
        else Prototypes found
            Node->>Node: Remove non-port fields
            Node->>Node: Add new fields from new category
            Node->>Node: Copy name and description if default
        end
    end
    Eagle->>Node: Set new category
    Eagle->>Eagle: Flag file as modified
    Eagle->>Eagle: Check graph consistency
Loading

File-Level Changes

Change Details Files
Modified the eligible node categories computation to work without built-in palettes
  • Removed dependency on input/output port count for category filtering
  • Simplified category type determination logic
  • Restructured the function to handle null selected nodes
src/Eagle.ts
Implemented fallback mechanism for category changes when built-in palette is unavailable
  • Added warning notification when built-in palette is not found
  • Wrapped template-based transformation logic in conditional block
  • Added direct category change as fallback behavior
  • Modified error handling to continue with basic category change instead of returning
src/Eagle.ts
Updated category listing utility to support offline operation
  • Removed input/output port count parameters
  • Added fallback to CategoryData.cData when built-in palette is not available
  • Modified return behavior to use buildComponentList instead of returning null
src/Utils.ts

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @james-strauss-uwa - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

src/Eagle.ts Show resolved Hide resolved
src/Eagle.ts Show resolved Hide resolved
Copy link
Collaborator

@M-Wicenec M-Wicenec left a comment

Choose a reason for hiding this comment

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

Its a little weird that the limitation is also an issue when simply using the setting to not load the palettes. Its not really clear to the user that using that setting will severely limit switching data nodes in that way when using it.

not sure if the correct thing to do would be to simply mention that in the tooltips or to instead have them loaded but hidden, so we can still use them for this purpose?

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.

2 participants