Skip to content

feat: add boundary for layer navigation#393

Merged
nk1408 merged 14 commits into
mainfrom
fix/additional-boundary-layers
Jul 23, 2025
Merged

feat: add boundary for layer navigation#393
nk1408 merged 14 commits into
mainfrom
fix/additional-boundary-layers

Conversation

@nk1408

@nk1408 nk1408 commented Jul 16, 2025

Copy link
Copy Markdown
Collaborator

Pull Request

Description

Implement a "no additional layer" boundary feedback system for navigation between layers

Related Issues

Closes #372

Changes Made

src/model/abstract.ts

  1. Made notifyOutOfBounds() public instead of protected to allow the Context class to call it directly

src/model/context.ts

  1. Simplified stepTrace() method by removing manual boundary checks and letting subplot handle boundary logic naturally
  2. Added boundary state handling to maintain context level when subplot doesn't move (boundary hit) by adding current trace back to context

src/model/plot.ts

  1. Added moveOnce() method to Subplot class to handle boundary logic through inheritance from AbstractObservableElement
  2. Enhanced state getter to return empty subplot state when isOutOfBounds is true (already existed)
    src/service/audio.ts
  3. Enhanced onStateChange() method to handle empty subplot states and play boundary audio using playEmptyTone(1, 0)
  4. Added audio overlap prevention with stopAll() calls before playing boundary audio to prevent multiple audio tracks

src/service/text.ts

  1. Modified format() method to display "No additional layer" instead of "No subplot info to display" for empty subplot states

Checklist

  • I have read the Contributor Guidelines.
  • I have performed a self-review of my own code and ensured it follows the project's coding standards.
  • I have tested the changes locally following ManualTestingProcess.md, and all tests related to this pull request pass.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation, if applicable.
  • I have added appropriate unit tests, if applicable.

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@nk1408

nk1408 commented Jul 16, 2025

Copy link
Copy Markdown
Collaborator Author

Looks like there is a version discrepancy b/w GH actions & local linter, will fix this along this feature

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@nk1408

nk1408 commented Jul 16, 2025

Copy link
Copy Markdown
Collaborator Author

regeneration of lock file resolved the lint version discrepancy

@nk1408 nk1408 requested a review from jooyoungseo July 16, 2025 23:57
@nk1408

nk1408 commented Jul 16, 2025

Copy link
Copy Markdown
Collaborator Author

braille.ts and navigation.ts changes are result of lint changes and aren't related to this feature

@jooyoungseo jooyoungseo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Test with examples/barplot.html. The boundary warning doesn't work in a single-layer plot.

@jooyoungseo jooyoungseo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It'd be great if you could cover this as part of this PR: In examples/multi-panel.html, you would see "No figure info to display" when you reach boundary. Please add the boundary sound here.

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@nk1408

nk1408 commented Jul 17, 2025

Copy link
Copy Markdown
Collaborator Author

@jooyoungseo , I've adressed both these changes. Please let me know if it is looking good. Thanks

@nk1408 nk1408 requested a review from jooyoungseo July 17, 2025 14:53
@jooyoungseo jooyoungseo changed the title fix: add boundary for layer navigation feat: add boundary for layer navigation Jul 17, 2025

@jooyoungseo jooyoungseo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When sound is off by pressing S key, the boundary sound must be turned off as well. This worked originally in main branch.

@nk1408

nk1408 commented Jul 17, 2025

Copy link
Copy Markdown
Collaborator Author

When sound is off by pressing S key, the boundary sound must be turned off as well. This worked originally in main branch.

sure, working on it, professor

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@jooyoungseo jooyoungseo requested a review from Copilot July 18, 2025 02:12

This comment was marked as outdated.

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@jooyoungseo jooyoungseo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Test with examples/barplot.html. It says "No plot info to display" instead of "No additional layer

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@nk1408

nk1408 commented Jul 18, 2025

Copy link
Copy Markdown
Collaborator Author

Test with examples/barplot.html. It says "No plot info to display" instead of "No additional layer

addressed this issue in commit 18b467a

@jooyoungseo jooyoungseo requested a review from Copilot July 19, 2025 20:31

This comment was marked as outdated.

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@jooyoungseo jooyoungseo requested a review from Copilot July 20, 2025 15:04

This comment was marked as outdated.

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@nk1408 nk1408 requested a review from jooyoungseo July 22, 2025 00:34
@jooyoungseo jooyoungseo requested a review from Copilot July 22, 2025 00:42

This comment was marked as outdated.

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@nk1408 nk1408 requested a review from Copilot July 23, 2025 16:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a boundary feedback system for navigation between layers in the multimodal access system. The changes add proper audio and text feedback when users attempt to navigate beyond available layers (e.g., "No additional layer" message).

  • Adds boundary detection utility for grid-based navigation
  • Implements moveOnce() methods to handle boundary logic in plot elements
  • Enhances audio and text services to provide appropriate feedback for boundary conditions

Reviewed Changes

Copilot reviewed 7 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/util/navigation.ts New utility function for boundary detection in grid navigation
src/service/text.ts Updates text formatting to display "No additional layer" for empty subplot states
src/service/audio.ts Enhances audio service to handle boundary conditions and prevent audio overlap
src/model/plot.ts Adds moveOnce() methods and trace interface updates for boundary handling
src/model/context.ts Simplifies trace stepping logic and adds boundary state handling
src/model/abstract.ts Makes notifyOutOfBounds() public and adds getId() method to AbstractTrace
src/controller.ts Moves audio service observer registration from subplot to figure level

Comment thread src/util/navigation.ts Outdated
Comment thread src/util/navigation.ts Outdated
Comment thread src/model/context.ts
Comment thread src/model/context.ts Outdated
@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@github-actions

Copy link
Copy Markdown

⚠️ Changes detected in model-related files

This PR contains changes to files that affect the model layer and unit tests. Before merging, please ensure:

  1. Run the unit tests locally:

    npm run test
  2. Update tests if necessary

@nk1408 nk1408 merged commit 3d36d32 into main Jul 23, 2025
5 checks passed
@nk1408 nk1408 deleted the fix/additional-boundary-layers branch July 23, 2025 21:30
@xabilitylab

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 3.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released For issues/features released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Add "no additional layer" boundary feedback for layer navigation

4 participants