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

Add support for virtual controller for Intellivision in Bliss #2357

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

proskd
Copy link
Contributor

@proskd proskd commented Oct 25, 2024

User description

What does this PR do

Adds virtual controller support for Intellivision (Bliss Core should now work ok)

Where should the reviewer start

How should this be manually tested

Any background context you want to provide

What are the relevant tickets

Screenshots (important for UI changes)

Questions


PR Type

enhancement


Description

  • Added support for a virtual controller for Intellivision in the Bliss emulator.
  • Implemented new button mappings and tags in the Intellivision controller view controller.
  • Updated control layout sizes and button configurations in system resource files.
  • Added new buttons with specific frames and titles, enhancing the virtual controller's functionality.

Changes walkthrough 📝

Relevant files
Enhancement
PVIntellivisionControllerViewController.swift
Implement virtual controller button mappings                         

PVUI/Sources/PVUIBase/Controller/Systems/PVIntellivisionControllerViewController.swift

  • Added button tag assignments for various buttons.
  • Updated button titles to match new tags.
  • Set selectButton and startButton tags to clear and enter.
  • +31/-10 
    systems.plist
    Update control layout and button configurations                   

    PVCoreLoader/Sources/PVCoreLoader/Resources/systems.plist

  • Modified control layout size.
  • Adjusted button frames and added new buttons.
  • Updated button titles and tints.
  • +113/-3 
    systems.plist
    Revise control layout and add new buttons                               

    PVLibrary/Sources/PVLibrary/Resources/systems.plist

  • Expanded control layout size.
  • Added multiple new buttons with specific frames.
  • Changed start and select button titles.
  • +141/-31

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Bug
    The button tag for '0' is set to .button9, which might be incorrect. It should probably be .button0 or a new tag for zero.

    Inconsistent Naming
    The button titles use a mix of letters and numbers. Consider using a consistent naming convention for all buttons.

    Duplicate Code
    The button configurations in this file are very similar to those in the PVCoreLoader/systems.plist file. Consider refactoring to avoid duplication.

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the button tag assignment for the "0" button

    The button tag for "0" is incorrectly set to .button9. Consider changing it to
    .button0 or a more appropriate tag.

    PVUI/Sources/PVUIBase/Controller/Systems/PVIntellivisionControllerViewController.swift [54-56]

     } else if (button.titleLabel?.text == "0") {
    -    button.buttonTag = .button9
    +    button.buttonTag = .button0
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a potential bug where the button tag for "0" is incorrectly set to .button9. Correcting this to .button0 is crucial for ensuring the correct functionality of the button, making this a high-impact fix.

    9
    Enhancement
    Refactor button tag assignment logic to improve code maintainability and readability

    Consider using a switch statement or a dictionary mapping for button tag assignments
    to improve readability and maintainability.

    PVUI/Sources/PVUIBase/Controller/Systems/PVIntellivisionControllerViewController.swift [30-56]

    -if button.titleLabel?.text == "L" {
    -    button.buttonTag = .bottomLeftAction
    -} else if (button.titleLabel?.text == "R") {
    -    button.buttonTag = .bottomRightAction
    -} else if (button.titleLabel?.text == "T") {
    -    button.buttonTag = .topAction
    -} else if button.titleLabel?.text == "1" {
    -    button.buttonTag = .button1
    -} else if (button.titleLabel?.text == "2") {
    -    button.buttonTag = .button2
    -} else if (button.titleLabel?.text == "3") {
    -    button.buttonTag = .button3
    -} else if button.titleLabel?.text == "4" {
    -    button.buttonTag = .button4
    -} else if (button.titleLabel?.text == "5") {
    -    button.buttonTag = .button5
    -} else if (button.titleLabel?.text == "6") {
    -    button.buttonTag = .button6
    -} else if button.titleLabel?.text == "7" {
    -    button.buttonTag = .button7
    -} else if (button.titleLabel?.text == "8") {
    -    button.buttonTag = .button8
    -} else if (button.titleLabel?.text == "9") {
    -    button.buttonTag = .button9
    -} else if (button.titleLabel?.text == "0") {
    -    button.buttonTag = .button9
    +let buttonTagMapping: [String: ButtonTag] = [
    +    "L": .bottomLeftAction,
    +    "R": .bottomRightAction,
    +    "T": .topAction,
    +    "1": .button1,
    +    "2": .button2,
    +    "3": .button3,
    +    "4": .button4,
    +    "5": .button5,
    +    "6": .button6,
    +    "7": .button7,
    +    "8": .button8,
    +    "9": .button9,
    +    "0": .button9
    +]
    +
    +if let buttonText = button.titleLabel?.text,
    +   let buttonTag = buttonTagMapping[buttonText] {
    +    button.buttonTag = buttonTag
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a dictionary mapping for button tag assignments significantly improves the readability and maintainability of the code by reducing repetitive conditional statements. This refactoring is beneficial for future modifications and understanding of the code.

    7

    💡 Need additional feedback ? start a PR chat

    } else if (button.titleLabel?.text == "9") {
    button.buttonTag = .button9
    } else if (button.titleLabel?.text == "0") {
    button.buttonTag = .button9
    Copy link
    Member

    Choose a reason for hiding this comment

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

    button 9 is twice

    @JoeMatt JoeMatt merged commit 8867787 into Provenance-Emu:develop-spm-2024 Oct 25, 2024
    2 of 3 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants