Skip to content

Fix display_name returning index instead of name on macOS - #147

Open
ralphweng2023 wants to merge 1 commit into
thoth-tech:mainfrom
ralphweng2023:fix/display-name-macos
Open

ralphweng2023 wants to merge 1 commit into
thoth-tech:mainfrom
ralphweng2023:fix/display-name-macos

Conversation

@ralphweng2023

Copy link
Copy Markdown

Description

This PR fixes the display_name() function returning display index numbers ("0", "1", "2") instead of actual display names on macOS.

Problem

On newer macOS versions, SDL_GetDisplayName() returns the display index as a string instead of the actual display name. This is a known SDL2 limitation.

Before:

Display Number: 1
Name: 0

After:

Display Number: 1
Name: Built-in Retina Display

Root Cause

SDL_GetDisplayName() in SDL2 does not properly retrieve display names on macOS 10.15+ (Catalina and later).

Solution

Use native macOS NSScreen.localizedName API via Objective-C++ to get proper display names. The implementation:

  1. Adds a new core_driver_macos.mm file with macOS-specific code
  2. Uses NSScreen.localizedName which returns proper names like "Built-in Retina Display" or "DELL U2720Q"
  3. Falls back to SDL_GetDisplayName() on older macOS versions (pre-10.15) or other platforms

Files Changed

  • core_driver.cpp: Added macOS-specific code path using sk_macos_get_display_name()
  • core_driver_macos.mm: New Objective-C++ file implementing the macOS NSScreen API
  • CMakeLists.txt: Updated to include .mm files in the macOS build

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  1. Created standalone test program using NSScreen.localizedName:
$ /tmp/test_macos_display
Number of screens: 1
Display 0: 'Built-in Retina Display'
  1. Built SplashKit on macOS with the fix:
cd projects/cmake
cmake --preset macOS
cmake --build build/
# Build succeeded

Testing Checklist

  • Code compiles without errors on macOS
  • Native macOS API returns correct display name
  • Falls back gracefully on older macOS versions
  • Tested on Linux (no changes to Linux path)
  • Tested on Windows (no changes to Windows path)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

Root cause: SDL_GetDisplayName returns display index ('0', '1', etc.)
instead of actual display names on newer macOS versions.

Solution: Use native macOS NSScreen.localizedName API via Objective-C++
to get proper display names like 'Built-in Retina Display'.

Changes:
- core_driver.cpp: Add macOS-specific code path using sk_macos_get_display_name()
- core_driver_macos.mm: New Objective-C++ file implementing NSScreen API
- CMakeLists.txt: Add macOS .mm files to build

Tested on macOS - now correctly returns 'Built-in Retina Display'
instead of '0'.

@connorclancydev connorclancydev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

General Information

This is pull is a fix for mac that lets mac correctly find the display name instead of integers

Code Quality

Code is well commented allowing people who are maintaining the code to understand what is done by the code, additionally as far as I can tell tha changes made successfully only target mac users not windows or linux. The code should be able to be changed easily in the future should it need to be as it is well commented and clear to read.

Functionality

When Tested on my Mac it seemed like the tests didnt fail I followed the instrutions for testing as written in the pull request and there were not any issues with the code returning the index instead of the name so I would say its functional

Testing

unit tests were not modified or made more of all the tests do seem to work with this change if more tests need to be added for this it can likely be added in a windows unit test or some other graphics testing motion, but this PR does fix the bug for the moment

Pull Request Details

The Pull request is successful in describing the changes and what to test so that people may replicate results to ensure that the change fixes the issue. The PR does successfully solve the issue which should assist MACOs Users. More effort might need to be done down the line to ensure longevity for MACos if updates they make deteriorate functions further but for the moment this PR is all good

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