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.

Sign up for GitHub

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

Jump to bottom

Refactor the TerminalDispatch graphics rendition to match AdaptDispatch #6725

Closed
j4james opened this issue Jun 30, 2020 · 1 comment · Fixed by #6728
Closed

Refactor the TerminalDispatch graphics rendition to match AdaptDispatch #6725

j4james opened this issue Jun 30, 2020 · 1 comment · Fixed by #6728
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Jun 30, 2020

Description of the new feature/enhancement

I'd like to propose we refactor the SetGraphicsRendition implementation in the TerminalDispatch class to match the refactoring done for AdaptDispatch in PR #5758.

I've been looking at adding a few more of the SGR attributes, but I found that adding them to the TerminalDispatch was a bit of a pain, because every new attribute required a new method in the ITerminalApi interface. If we matched the AdaptDispatch implementation, it would just be one more entry in the switch statement.

Other benefits are that the AdaptDispatch implementation is a bit simpler, and easier to follow. It should be a little more efficient when dealing with multiple SGR options. And ultimately we're going to need these implementations to be identical anyway, so we can deduplicate them for issue #3849.

Proposed technical implementation details (optional)

  1. Add GetTextAttributes and SetTextAttributes methods to the ITerminalApi interface to match the PrivateGetTextAttributes and PrivateSetTextAttributes methods in the ConGetSet interface.
  2. Essentially just copy and paste the AdaptDispatch::SetGraphicsRendition implementation into TerminalDispatch. I think the only difference is we don't need the color index remapping, and the color index constants are different for the same reason (resolving that discrepancy is still on my TODO list).
  3. Delete all the methods in the ITerminalApi interface that are no longer required.
@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 30, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 30, 2020
@j4james j4james mentioned this issue Jun 30, 2020
Merged
4 tasks
@ghost ghost added the In-PR This issue has a related PR label Jun 30, 2020
@j4james
Copy link
Collaborator Author

j4james commented Jun 30, 2020

As you can see, I've already gone ahead with a PR for this, just because it was such a trivial change. If you don't like it, though, you can always reject it.

@ghost ghost removed the In-PR This issue has a related PR label Jul 1, 2020
DHowett pushed a commit that referenced this issue Jul 1, 2020
@j4james
Refactor TerminalDispatch (graphics) to match AdaptDispatch (#6728)
6b43ace 
This is essentially a rewrite of the
`TerminalDispatch::SetGraphicsRendition` method, bringing it into closer
alignment with the `AdaptDispatch` implementation, simplifying the
`ITerminalApi` interface, and making the code easier to extend. It adds
support for a number of attributes which weren't previously implemented.

REFERENCES

* This is a mirror of the `AdaptDispatch` refactoring in PR #5758.
* The closer alignment with `AdaptDispatch` is a small step towards
  solving issue #3849.
* The newly supported attributes should help a little with issues #5461
  (italics) and #6205 (strike-through).

DETAILS

I've literally copied and pasted the `SetGraphicsRendition`
implementation from `AdaptDispatch` into `TerminalDispatch`, with only
few minor changes:

* The `SetTextAttribute` and `GetTextAttribute` calls are slightly
  different in the `TerminalDispatch` version, since they don't return a
  pointless `success` value, and in the case of the getter, the
  `TextAttribute` is returned directly instead of by reference.
  Ultimately I'd like to move the `AdaptDispatch` code towards that way
  of doing things too, but I'd like to deal with that later as part of a
  wider refactoring of the `ConGetSet` interface.
* The `SetIndexedForeground256` and `SetIndexedBackground256` calls
  required the color indices to be remapped in the `AdaptDispatch`
  implementation, because the conhost color table is in a different
  order to the XTerm standard. `TerminalDispatch` doesn't have that
  problem, so doesn't require the mapping.
* The index color constants used in the 16-color `SetIndexedForeground`
  and `SetIndexedBackground` calls are also slightly different for the
  same reason.

VALIDATION

I cherry-picked this code on top of the #6506 and #6698 PRs, since
that's only way to really get the different color formats passed-through
to the terminal. I then ran a bunch of manual tests with various color
coverage scripts that I have, and confirmed that all the different color
formats were being rendered as expected.

Closes #6725
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor TerminalDispatch (graphics) to match AdaptDispatch j4james/terminal
1 participant
@j4james