Repository: harfbuzz/uharfbuzz (language: python)

Issue:
(Un)setting features for part of a buffer: what could that look like?
We currently don't support setting or unsetting of features for only part of the input buffer.

I don't yet understand the underlying HB calls needed to achieve this, and I also don't yet see what such an API could look like from the Python side. Could it be an operation on the buffer?


Discussion / hints:
`hb_shape()` API, just set `hb_feature_t`. The `start`/`end` ranges in `hb_feature_t` refer to cluster values. Depends on how you are converting Python string to add to HB buffer. If you do it this way:

https://github.com/harfbuzz/harfbuzz/blob/master/src/sample.py#L30

Then you can use Python string indices as your `start`/`end`. A value of 0 turns feature off.
Currently uharfbuzz’s `shape()` takes features as a dictionary of `{tag: bool}` which is unfortunate, but it uses `hb_feature_from_string()` to parse the tag part, so you can workaround this by using the extended (Python-like) syntax for the tag (`"tag[start:end]"`) and it should achieve the same as the C API.
e.g.:
```Python
shape(font, buffer, {"dlig[3:5]": True, "hlig[0:7]": True, "liga[0:7]": False})
```
Oh wow, it pretty much already works, I had no idea! Thanks.

I think two things need to change:

- the bool dict value is already interpreted as an int: the features typedef should be changed accordingly
- in addition to a dict, it should also accept a list of tuples

I'll try to prepare a PR later this week.
Before:
`features: Dict[str, bool] = None`
After:
`features: Union[Dict[str, int], List[Tuple[str, int]]] = None`
By `List[Tuple[str, int]]]` you mean providing the features for *each* character?

I advise you to look more into the index/cluster values. Because if they don't match between Python string indices and what is passed to HarfBuzz it will be a problem. See my other comment. The `uharfbuzz` code uses different (utf8/utf16/codepoints) API depending on internal CPython storage of the string, NOT the user-facing indices in the string. Is that a problem? Probably not. But want to make sure. Do we not care about 2byte Python runtime anymore? If that's true, the code might be okay.

I advise you also look into using Python `Slice` objects if that simplifies things. And you probably don't need to use `hb_feature_from_string()` anyway.
No I simply meant that in addition of accepting a `{'liga': True}` feature dict, to allow `[('liga': True)]`, and just keep on exposing the `hb_feature_from_string()` syntax. It's just that a dict doesn't make sense anymore once you do `[('aalt[3]', 2)]`.

Although I realize now that it's a bit of a hybrid that we expose: the value is an explicit value, but the slice is parsed from the tag string. For me, using `hb_feature_from_string()` is good enough, but I'll think a bit more: perhaps it's indeed nicer to do something with `hb_feature_t` directly instead of going through `hb_feature_from_string()`.

I'm not sure I completely follow the consequences of the clusters vs indices issue. (I personally don't care much about the 2byte Python runtime anymore.) Are clusters equivalent to Python str indices on a 4byte Python build?
> No I simply meant that in addition of accepting a `{'liga': True}` feature dict, to allow `[('liga': True)]`, and just keep on exposing the `hb_feature_from_string()` syntax. It's just that a dict doesn't make sense anymore once you do `[('aalt[3]', 2)]`.
> 
> Although I realize now that it's a bit of a hybrid that we expose: the value is an explicit value, but the slice is parsed from the tag string. For me, using `hb_feature_from_string()` is good enough, but I'll think a bit more: perhaps it's indeed nicer to do something with `hb_feature_t` directly instead of going through `hb_feature_from_string()`.

This is the syntax I propose, by way of example:
`{'liga': True, 'kern': (slice(1,2)), 'feat': {slice(1,2):False}}`

> I'm not sure I completely follow the consequences of the clusters vs indices issue. (I personally don't care much about the 2byte Python runtime anymore.) Are clusters equivalent to Python str indices on a 4byte Python build?

Depends. The code is this: 
```

    def add_str(self, text: str,
                item_offset: int = 0, item_length: int = -1) -> None:
        # ensure unicode string is in the "canonical" representation
        assert PyUnicode_IS_READY(text)

        cdef Py_ssize_t length = PyUnicode_GET_LENGTH(text)
        cdef int kind = PyUnicode_KIND(text)

        if kind == PyUnicode_1BYTE_KIND:
            hb_buffer_add_latin1(
                self._hb_buffer,
                <uint8_t*>PyUnicode_1BYTE_DATA(text),
                length,
                item_offset,
                item_length,
            )
        elif kind == PyUnicode_2BYTE_KIND:
            hb_buffer_add_utf16(
                self._hb_buffer,
                <uint16_t*>PyUnicode_2BYTE_DATA(text),
                length,
                item_offset,
                item_length,
            )
        elif kind == PyUnicode_4BYTE_KIND:
            hb_buffer_add_utf32(
                self._hb_buffer,
                <uint32_t*>PyUnicode_4BYTE_DATA(text),
                length,
                item_offset,
                item_length,
            )
        else:
            raise AssertionError(kind)
```
I don't know the internals of CPython enough to know if the `1BYTE_KIND`, `2BYTE_KIND`, `4BYTE_KIND` map to Latin1-only, UCS2-only, UCS4. If they do (and I'm *guessing* they do), then this code is fine.

The `add_codepoints()` and `add_utf8` seem fine.
> I don't know the internals of CPython enough to know if the `1BYTE_KIND`, `2BYTE_KIND`, `4BYTE_KIND` map to Latin1-only, UCS2-only, UCS4. If they do (and I'm _guessing_ they do), then this code is fine.

Okay reading the docs:
```
Return a pointer to the canonical representation cast to UCS1, UCS2 or UCS4 integer types for direct character access. No checks are performed if the canonical representation has the correct character size; use PyUnicode_KIND() to select the right macro. Make sure PyUnicode_READY() has been called before accessing this.
```
I think this is good.

[I don't think `UCS1` is a real term.]
> This is the syntax I propose, by way of example:
{'liga': True, 'kern': (slice(1,2)), 'feat': {slice(1,2):False}}

Ok, that makes sense. Do you mean the value can be a sequence of slice objects _or_ a single slice object? Perhaps better if it has to be a sequence. `{slice(1,2):False}` won't work as slices are strangely enough not hashable. A list of tuples is the next best thing I guess.

Let me reiterate, and see whether I understand you correctly:

1. What we pass to the `features` argument of `shape()` remains a dict, with feature tags as keys
2. Values can be
    - bool
    - int (for `aalt`-style alt selection)
    - a sequence of `slice()` objects (implying the feature value should be set to True for the slices)
    - a sequence `(slice(), value)` tuples, where `value` is bool or int.
> Perhaps better if it has to be a sequence. `{slice(1,2):False}` won't work as slices are strangely enough not hashable.

Ouch. Complete surprise!

> A list of tuples is the next best thing I guess.

Sure that's the remedy...

> * What we pass to the `features` argument of `shape()` remains a dict, with feature tags as keys
> * Values can be
>   
>   * bool
>   * int (for `aalt`-style alt selection)
>   * a sequence of `slice()` objects (implying the feature value should be set to True for the slices)
>   * a sequence `(slice(), value)` tuples, where `value` is bool or int.

Right. That looks good to me given the constraints. To me, moving the optional slice to the key side of things, ie. key being either a tag or a `(tag, slice)` makes more sense but as you said slices are not hashable...
`slice()` also allows for `step` and negative values, neither of which add something useful here I would think. Does the HB slice syntax allow for negative indices?

(I can totally see us simply using `(start, stop)` tuples instead of `slice()` objects.)

> Right. That looks good to me given the constraints. To me, moving the optional slice to the key side of things, ie. key being either a tag or a (tag, slice) makes more sense but as you said slices are not hashable...

Once the key is no longer purely a tag, I'm not sure a dict makes much sense anymore.
> `slice()` also allows for `step` and negative values, neither of which add something useful here I would think. Does the HB slice syntax allow for negative indices?
>
> (I can totally see us simply using `(start, stop)` tuples instead of `slice()` objects.)

Ack. Only simply `(start, end)` slices make sense.

> Once the key is no longer purely a tag, I'm not sure a dict makes much sense anymore.

True. Specially since later values override earlier ones in HarfBuzz if there is overlap in ranges for the same tag.
Ok, then how about this:

1. What we pass to the features argument of shape() remains a dict, with feature tags as keys.
2. Values can be:
    - bool
    - int (for aalt-style alt selection)
    - a sequence of 3-tuples: `(start, stop, value)`
> Ok, then how about this:
> 
> 1. What we pass to the features argument of shape() remains a dict, with feature tags as keys.
> 2. Values can be:
>    
>    * bool
>    * int (for aalt-style alt selection)
>    * a sequence of 3-tuples: `(start, stop, value)`

That works.
Awesome, I'll try to turn this into a PR.
> Awesome, I'll try to turn this into a PR.

Great.

Thank you again for embracing HarfBuzz. :)
HarfBuzz embraced me :)

The features arg will become:

`features: Dict[str,Union[int,bool,Sequence[Tuple[int,int,Union[int,bool]]]]] = None`
Lovely. Thanks.

Fix patch:
diff --git a/src/uharfbuzz/_harfbuzz.pyx b/src/uharfbuzz/_harfbuzz.pyx
index bbe3ee5..560f0b5 100644
--- a/src/uharfbuzz/_harfbuzz.pyx
+++ b/src/uharfbuzz/_harfbuzz.pyx
@@ -3,7 +3,7 @@ from enum import IntEnum
 from .charfbuzz cimport *
 from libc.stdlib cimport free, malloc
 from libc.string cimport const_char
-from typing import Callable, Dict, List, Tuple
+from typing import Callable, Dict, List, Sequence, Tuple, Union
 
 
 cdef extern from "Python.h":
@@ -579,34 +579,50 @@ cdef const char ** to_cstring_array(list_str):
     return ret
 
 
-def shape(font: Font, buffer: Buffer, features: Dict[str, bool] = None,
+def shape(font: Font, buffer: Buffer,
+        features: Dict[str,Union[int,bool,Sequence[Tuple[int,int,Union[int,bool]]]]] = None,
         shapers: List[str] = None) -> None:
     cdef unsigned int size
     cdef hb_feature_t* hb_features
     cdef bytes packed
-    cdef char* cstr
     cdef hb_feature_t feat
     cdef const char **c_shapers
-    if features is None:
-        size = 0
-        hb_features = NULL
-    else:
-        size = len(features)
-        hb_features = <hb_feature_t*>malloc(size * sizeof(hb_feature_t))
-        for i, (name, value) in enumerate(features.items()):
-            packed = name.encode()
-            cstr = packed
-            hb_feature_from_string(packed, len(packed), &feat)
-            feat.value = value
-            hb_features[i] = feat
-    if shapers:
-        c_shapers = to_cstring_array(shapers)
-        hb_shape_full(font._hb_font, buffer._hb_buffer, hb_features, size, c_shapers)
-        free(c_shapers)
-    else:
-        hb_shape(font._hb_font, buffer._hb_buffer, hb_features, size)
-    if hb_features is not NULL:
-        free(hb_features)
+    size = 0
+    hb_features = NULL
+    try:
+        if features:
+            for value in features.values():
+                if isinstance(value, int):
+                    size += 1
+                else:
+                    size += len(value)
+            hb_features = <hb_feature_t*>malloc(size * sizeof(hb_feature_t))
+            i = 0
+            for name, value in features.items():
+                assert i < size, "index out of range for feature array capacity"
+                packed = name.encode()
+                if isinstance(value, int):
+                    hb_feature_from_string(packed, len(packed), &feat)
+                    feat.value = value
+                    hb_features[i] = feat
+                    i += 1
+                else:
+                    feat.tag = hb_tag_from_string(packed, -1)
+                    for start, end, value in value:
+                        feat.value = value
+                        feat.start = start
+                        feat.end = end
+                        hb_features[i] = feat
+                        i += 1
+        if shapers:
+            c_shapers = to_cstring_array(shapers)
+            hb_shape_full(font._hb_font, buffer._hb_buffer, hb_features, size, c_shapers)
+            free(c_shapers)
+        else:
+            hb_shape(font._hb_font, buffer._hb_buffer, hb_features, size)
+    finally:
+        if hb_features is not NULL:
+            free(hb_features)
 
 
 DEF STATIC_TAGS_ARRAY_SIZE = 128


Test patch:
diff --git a/tests/test_uharfbuzz.py b/tests/test_uharfbuzz.py
index 8ecfcbe..f365076 100644
--- a/tests/test_uharfbuzz.py
+++ b/tests/test_uharfbuzz.py
@@ -196,6 +196,25 @@ def test_glyh_name_no_features(self, blankfont, string, expected):
         assert glyph_names == expected
         assert blankfont.glyph_to_string(1000) == 'gid1000'
 
+    @pytest.mark.parametrize(
+        "string, features, expected",
+        [
+            # The calt feature replaces c by a in the context e, d, c', b, a.
+            ("edcbaedcba", {}, ["e", "d", "a", "b", "a", "e", "d", "a", "b", "a"]),
+            ("edcbaedcba", {"calt[2]": False}, ["e", "d", "c", "b", "a", "e", "d", "a", "b", "a"]),
+            ("edcbaedcba", {"calt": [(7, 8, False)]}, ["e", "d", "a", "b", "a", "e", "d", "c", "b", "a"]),
+            ("edcbaedcba", {"calt": [(0, 10, False), (7, 8, True)]}, ["e", "d", "c", "b", "a", "e", "d", "a", "b", "a"]),
+        ],
+    )
+    def test_features_slice(self, blankfont, string, features, expected):
+        buf = hb.Buffer()
+        buf.add_str(string)
+        buf.guess_segment_properties()
+        hb.shape(blankfont, buf, features)
+
+        glyph_names = [blankfont.glyph_to_string(g.codepoint) for g in buf.glyph_infos]
+        assert glyph_names == expected
+
 
 class TestCallbacks:
     def test_nominal_glyph_func(self, blankfont):