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

[RFE#1736] feat: Alternative translation highlight #950

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

chelobaka
Copy link
Contributor

Pull request type

Please mark github LABEL of the type of change your PR introduces:

  • Feature enhancement -> [enhancement]

Which ticket is resolved?

What does this PR change?

  • This PR implements new highlighter for marking segments with an alternative translation.
  • GUI bits one might expect for a highlighter like changing color in settings and default color values for light/dark themes.

Other information

@miurahr
Copy link
Member

miurahr commented Feb 7, 2024

OmegaT marker plugins implementations and marker API are incomplete.
When we want to extend something related to marker API, it is chance to refactor and enhance marker API.
One of places we should improve is marker preference.
We should allow marker plugin to extend preference without changing IEditorSettings class and also extending UI parts.
IMarker should provide a way to give preference API that marker implementation just return a name of marker, preference key and default values.

@miurahr miurahr self-requested a review February 7, 2024 00:38
@t-cordonnier
Copy link
Contributor

About marking alternative translations, let's remember that I had another proposal:
https://github.com/t-cordonnier/omegat/tree/mark-tra-alternative

In this proposal, alternative translations are marked two ways:

  • as a context variable in translation info => advantage: the mark is visible not only for current segment, but also other ones
  • as an extra character in the segment marker (only because I know some people wanted it)

The problem with using a marker is that you have to think about accumulation: what if the segment is alternative + coming from tm/auto?

@chelobaka
Copy link
Contributor Author

OmegaT marker plugins implementations and marker API are incomplete. When we want to extend something related to marker API, it is chance to refactor and enhance marker API. One of places we should improve is marker preference. We should allow marker plugin to extend preference without changing IEditorSettings class and also extending UI parts. IMarker should provide a way to give preference API that marker implementation just return a name of marker, preference key and default values.

Sounds reasonable. Shall I continue with this PR the old way or the interfaces should be extended first? And I'd be happy to help if you need it.

@miurahr
Copy link
Member

miurahr commented Feb 7, 2024

OmegaT marker plugins implementations and marker API are incomplete. When we want to extend something related to marker API, it is chance to refactor and enhance marker API. One of places we should improve is marker preference. We should allow marker plugin to extend preference without changing IEditorSettings class and also extending UI parts. IMarker should provide a way to give preference API that marker implementation just return a name of marker, preference key and default values.

Sounds reasonable. Shall I continue with this PR the old way or the interfaces should be extended first? And I'd be happy to help if you need it.

The work is proposed in #951
An improvement requires hard work to change many, so you can do it with old way, and I'll propose the change with your improvement in a place of configurations and menu items.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Bundle.properties changes are fine.

I'll leave comments on the code itself to people who actually know what they're doing! 😄
Nice to see a new name among people submitting a pull request!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not new exactly, but rarely seen for sure 😄

@miurahr

This comment was marked as resolved.

@miurahr

This comment was marked as resolved.

@miurahr
Copy link
Member

miurahr commented Feb 10, 2024

Here is patches files omegat-alttranslationsmarker-patches.zip for your convenience.

@miurahr miurahr added this to the 6.1.0 (Require Java 11) milestone Feb 10, 2024
miurahr and others added 10 commits February 10, 2024 12:59
* refactor: whitespace marker to be single method

This reduces a number of threads and may improve performance. It also help make WhitespaceMarkers to be implemented as plugins.

Signed-off-by: Hiroshi Miura <[email protected]>

* feat: register WhitespaceMarker as plugin

Signed-off-by: Hiroshi Miura <[email protected]>

* feat: WhitespaceMarker unit test

Signed-off-by: Hiroshi Miura <[email protected]>

* fix: typo

Signed-off-by: Hiroshi Miura <[email protected]>

* feat: improve BiDi marker unit test

Signed-off-by: Hiroshi Miura <[email protected]>

* fix: spotbugs errors

Signed-off-by: Hiroshi Miura <[email protected]>

* style: copyright header

Signed-off-by: Hiroshi Miura <[email protected]>

* fix: condition bugs

- Fix wrong status check
- mark when segment is not active

Signed-off-by: Hiroshi Miura <[email protected]>

* fix: condition bugs

- Fix wrong status check
- mark when segment is not active

Signed-off-by: Hiroshi Miura <[email protected]>

* refactor: improve test assertion

Signed-off-by: Hiroshi Miura <[email protected]>

* chore: update copyright holder

Signed-off-by: Hiroshi Miura <[email protected]>

---------

Signed-off-by: Hiroshi Miura <[email protected]>
* refactor: make filters2 to be omegat3 plugin style

Signed-off-by: Hiroshi Miura <[email protected]>

* style: fix errors for filters

Signed-off-by: Hiroshi Miura <[email protected]>

---------

Signed-off-by: Hiroshi Miura <[email protected]>
This make StaX filters as new style plugins by adding loadPlugins() and unloadPlugins() functions.

Signed-off-by: Hiroshi Miura <[email protected]>
omegat-org#908)

* docs_devel: update document contribution manual

Signed-off-by: Hiroshi Miura <[email protected]>

* docs_devel: Fix link URLs

Signed-off-by: Hiroshi Miura <[email protected]>

* docs_devel: Split manual contribution guide and build manual

Signed-off-by: Hiroshi Miura <[email protected]>

* docs: update doc_src/Readme.md

Remove duplication with the contribution guide, and replace with links to the guide

Signed-off-by: Hiroshi Miura <[email protected]>

* docs_devel: explain about build task and how to skip manual generation

Signed-off-by: Hiroshi Miura <[email protected]>

* Update Readme.md

A few rewording suggestions.

* Update 02.HowToBuild.md

Various changes.

I put comments preceded by "->".

* Update 02.HowToBuild.md

Few more fixes.

* Update 07.ManualContainerTaskBuild.md

I removed most of the promotion for Docker and restructured the document a bit.

I think most of the last part is irrelevant now that we use containers.

I've added comments prefixed with "->".

* Update 40.ContributingDocument.md

Added reference to contribution rules, etc.

* Update 45.LocalizeApplicationAndManuals.md

Add the link to Kos' explanations.

* Update 92.CodeSigning.md

Small modifications

* Update 93.BuildingInstallerPackage.md

Various small fixes and a comment (check "->")

* chore: Update 02.HowToBuild.md

* chore: Update 02.HowToBuild.md

Fix typo and slight rewording.

* chore: Update 40.ContributingDocument.md

Wording change, because documentation isn't really "developed". :)

* chore: Update 45.LocalizeApplicationAndManuals.md

Wording tweaks.

* chore: Update 92.CodeSigning.md

* chore: Update 93.BuildingInstallerPackage.md

Various wording tweaks.

* chore: Update 93.BuildingInstallerPackage.md

Rewording of section on building Linux native packages using `jpackage`

* chore: Apply suggestions from code review

Co-authored-by: Hiroshi Miura <[email protected]>

* chore: Delete resolved inline comment in 93.BuildingInstallerPackage.md

* docs_devel: split document build manual

Signed-off-by: Hiroshi Miura <[email protected]>

---------

Signed-off-by: Hiroshi Miura <[email protected]>
Co-authored-by: Jean-Christophe Helary <[email protected]>
Co-authored-by: kazephil <[email protected]>
* refactor: fix marker test package and classname

Signed-off-by: Hiroshi Miura <[email protected]>

* chore: add ComesFromAutoTMMarkerTest test

Signed-off-by: Hiroshi Miura <[email protected]>

* chore: improve marker tests

Signed-off-by: Hiroshi Miura <[email protected]>

* feat: add ProtectedPartsMarker test

- Extend IEditor to have isOrientationAllLtr method to test the marker

Signed-off-by: Hiroshi Miura <[email protected]>

* feat: add ReplaceMarkerTest

- Extend MarkerTestBase to allow extend mock project configuration

Signed-off-by: Hiroshi Miura <[email protected]>

* refactor: ReplaceMarkerTest

Signed-off-by: Hiroshi Miura <[email protected]>

* style: ReplaceMarkerTest

- fix visibility of internal class

Signed-off-by: Hiroshi Miura <[email protected]>

* style: IEditor

Signed-off-by: Hiroshi Miura <[email protected]>

* fix: avoid interference among tests

RemoveTagMarkerTest does not change preference

Signed-off-by: Hiroshi Miura <[email protected]>

* chore: update jacoco target packages

Signed-off-by: Hiroshi Miura <[email protected]>

---------

Signed-off-by: Hiroshi Miura <[email protected]>
* refactor: markers: avoid unnecessary static

There are unnecessary static variable initialization and static functions. These break registering Markers as plugins.

Signed-off-by: Hiroshi Miura <[email protected]>

* refactor: initialize fields in constructor

Signed-off-by: Hiroshi Miura <[email protected]>

---------

Signed-off-by: Hiroshi Miura <[email protected]>
…de (omegat-org#945)

* [BUGS#1247] fix: filters4: create event reader when entering event mode

- Complete solution to avoid StaX implementation difference.

Signed-off-by: Hiroshi Miura <[email protected]>

* style: improve filters4 styles

- AbstractXmlFilter: variable name, case-default
- OpenXmlFilter: variable name, suppression update

Signed-off-by: Hiroshi Miura <[email protected]>

---------

Signed-off-by: Hiroshi Miura <[email protected]>
@chelobaka
Copy link
Contributor Author

Marker was registered in Plugin.properties as a marker. Moved it to plugins and added test files. Thank you!

@t-cordonnier
Copy link
Contributor

Just did a quick test, the plugin works well but as I suspected there is the problem of accumulation: if I take a segment from tm/auto, and this segment is an alternative translation, then I see only the result of your plugin, the color saying that this comes from tm/auto is recovered by yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants