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

console: move ui to libtrx #1541

Merged
merged 9 commits into from
Sep 26, 2024
Merged

console: move ui to libtrx #1541

merged 9 commits into from
Sep 26, 2024

Conversation

rr-
Copy link
Collaborator

@rr- rr- commented Sep 25, 2024

Checklist

  • I have read the coding conventions
  • I have added a changelog entry about what my pull request accomplishes, or it is an internal change

Description

LostArtefacts/libtrx#37

@rr- rr- added the Internal The invisible stuff label Sep 25, 2024
@rr- rr- added this to the 4.5 milestone Sep 25, 2024
@rr- rr- self-assigned this Sep 25, 2024
@rr- rr- requested review from a team as code owners September 25, 2024 20:18
@rr- rr- requested review from lahm86, walkawayy and aredfan and removed request for a team September 25, 2024 20:18
@rr- rr- force-pushed the libtrx-console branch 2 times, most recently from 082c783 to 7abc1fc Compare September 25, 2024 21:17
@aredfan
Copy link
Collaborator

aredfan commented Sep 25, 2024

The prior issues are now resolved, thank you. I've noticed a small issue - the blinking caret is slower now.

Copy link
Collaborator

@aredfan aredfan left a comment

Choose a reason for hiding this comment

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

The blinking caret LGTM. 👍

@lahm86
Copy link
Collaborator

lahm86 commented Sep 25, 2024

I have text scale set at 0.8, and the console shows here. It moves further up the lower the value and can go off-screen if too high.
image

@rr-
Copy link
Collaborator Author

rr- commented Sep 25, 2024

Fixed, but I can't say I like my solution. @lahm86 could you please take a look around the code, especially libtrx, and LMK if you see anything that I could've done better?

Copy link
Collaborator

@walkawayy walkawayy left a comment

Choose a reason for hiding this comment

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

LGTM at all text scalings

@rr-
Copy link
Collaborator Author

rr- commented Sep 26, 2024

Fixed, but I can't say I like my solution. @lahm86 could you please take a look around the code, especially libtrx, and LMK if you see anything that I could've done better?

I've improved the APIs a bit and I'm a bit happier. There are still some design smells such as UI having to be initialized after the config or else reacting to text scaling changes will not work, or the config change events being fired by Config_Write calls, but I was unable to come up with a reasonable alternative. The widgets, especially the stack, should be in a good spot now, though.

@rr- rr- requested a review from aredfan September 26, 2024 15:14
@rr- rr- merged commit e1cff64 into develop Sep 26, 2024
6 checks passed
@rr- rr- deleted the libtrx-console branch September 26, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal The invisible stuff TR1 TR2
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants