xkb: Fix use of nKTLevels

Submitted by Ran Benita on April 29, 2014, 3:39 p.m.

Details

Message ID 20140429153900.GA28231@ran
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Ran Benita April 29, 2014, 3:39 p.m.
Hi Peter

On Mon, Apr 28, 2014 at 05:05:10PM -0400, Peter Harris wrote:
> nLevelsPerType is nLevels long, and ktLevelNames is nKTLevels long.

Can you split the nKTLevels -> nTypes changes to a different commit?
They obviously seem correct to me, so for those:
Reviewed-by: Ran Benita <ran234@gmail.com>
Tested-by: Ran Benita <ran234@gmail.com>

As for the other changes, why do we want to get rid of sumof? Spec says
    l     LISTofCARD8 nLevelsPerType, sum of all elements=L"
so it seems appropriate to use it.

Also, unfortunately, they cause this diff:


The function xcb_xkb_get_names_value_list_unpack() is used in some
places (e.g. xkbcommon), so can't change it without API break.

BTW: when I add the missing param the xkbcommon tests do pass.

Ran

> This continues the work started in
> 76ca2c0b1527541be59c344118c538ba055ad9d8.
> 
> Signed-off-by: Peter Harris <pharris@opentext.com>
> ---
> 
> This removes the only usages of <sumof>. I debated removing it (or at
> least marking it deprecated in a comment) in the xsd, but a comment in
> xinput implies that it could be useful there in the future.
> 
>  src/xkb.xml | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/xkb.xml b/src/xkb.xml
> index 9ef4402..dbd219b 100644
> --- a/src/xkb.xml
> +++ b/src/xkb.xml
> @@ -1678,7 +1678,7 @@ authorization from the authors.
>  					    </op>
>  					</list>
>  					<list name="ktLevelNames" type="ATOM">
> -						<sumof ref="nLevelsPerType" />
> +						<fieldref>nKTLevels</fieldref>
>  					</list>
>  				</bitcase>
>  				<bitcase>
> @@ -1778,10 +1778,10 @@ authorization from the authors.
>  			<bitcase>
>  				<enumref ref="NameDetail">KTLevelNames</enumref>
>  				<list name="nLevelsPerType" type="CARD8">
> -					<fieldref>nKTLevels</fieldref>
> +					<fieldref>nTypes</fieldref>
>  				</list>
>  				<list name="ktLevelNames" type="ATOM">
> -					<sumof ref="nLevelsPerType" />
> +					<fieldref>nKTLevels</fieldref>
>  				</list>
>  			</bitcase>
>  			<bitcase>
> @@ -2220,10 +2220,10 @@ authorization from the authors.
>  						<bitcase>
>  							<enumref ref="NameDetail">KTLevelNames</enumref>
>  							<list name="nLevelsPerType" type="CARD8">
> -								<fieldref>nKTLevels</fieldref>
> +								<fieldref>nTypes</fieldref>
>  							</list>
>  							<list name="ktLevelNames" type="ATOM">
> -								<sumof ref="nLevelsPerType" />
> +								<fieldref>nKTLevels</fieldref>
>  							</list>
>  						</bitcase>
>  						<bitcase>
> -- 
> 1.9.2
> 
> _______________________________________________
> Xcb mailing list
> Xcb@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb

Patch hide | download patch | download mbox

diff --git a/src/xkb.h b/src/xkb.h
index 845d1c9..3820bb1 100644
--- a/src/xkb.h
+++ b/src/xkb.h
@@ -9371,6 +9371,7 @@  xcb_xkb_get_names_value_list_radio_group_names_end (const xcb_xkb_get_names_repl
 int
 xcb_xkb_get_names_value_list_serialize (void                                 **_buffer  /**< */,
                                         uint8_t                                nTypes  /**< */,
+                                        uint16_t                               nKTLevels  /**< */,
                                         uint32_t                               indicators  /**< */,
                                         uint16_t                               virtualMods  /**< */,
                                         uint8_t                                groupNames  /**< */,
@@ -9383,6 +9384,7 @@  xcb_xkb_get_names_value_list_serialize (void                                 **_
 int
 xcb_xkb_get_names_value_list_unpack (const void                      *_buffer  /**< */,
                                      uint8_t                          nTypes  /**< */,
+                                     uint16_t                         nKTLevels  /**< */,
                                      uint32_t                         indicators  /**< */,
                                      uint16_t                         virtualMods  /**< */,
                                      uint8_t                          groupNames  /**< */,
@@ -9395,6 +9397,7 @@  xcb_xkb_get_names_value_list_unpack (const void                      *_buffer  /
 int
 xcb_xkb_get_names_value_list_sizeof (const void  *_buffer  /**< */,
                                      uint8_t      nTypes  /**< */,
+                                     uint16_t     nKTLevels  /**< */,
                                      uint32_t     indicators  /**< */,
                                      uint16_t     virtualMods  /**< */,
                                      uint8_t      groupNames  /**< */,

Comments

On 2014-04-29 11:39, Ran Benita wrote:
> Hi Peter
> 
> On Mon, Apr 28, 2014 at 05:05:10PM -0400, Peter Harris wrote:
>> nLevelsPerType is nLevels long, and ktLevelNames is nKTLevels long.
> 
> Can you split the nKTLevels -> nTypes changes to a different commit?
> They obviously seem correct to me, so for those:
> Reviewed-by: Ran Benita <ran234@gmail.com>
> Tested-by: Ran Benita <ran234@gmail.com>
> 
> As for the other changes, why do we want to get rid of sumof?

It's one more thing that every single generator has to implement. I'd
like to keep the number of things to a minimum if at all possible.

> Spec says
>     l     LISTofCARD8 nLevelsPerType, sum of all elements=L"
> so it seems appropriate to use it.

The spec is wrong. See the comment added to xkb.xml in
76ca2c0b1527541be59c344118c538ba055ad9d8.

> Also, unfortunately, they cause this diff:
> 
> diff --git a/src/xkb.h b/src/xkb.h
> index 845d1c9..3820bb1 100644
> --- a/src/xkb.h
> +++ b/src/xkb.h
> @@ -9371,6 +9371,7 @@ xcb_xkb_get_names_value_list_radio_group_names_end (const xcb_xkb_get_names_repl
>  int
>  xcb_xkb_get_names_value_list_serialize (void                                 **_buffer  /**< */,
>                                          uint8_t                                nTypes  /**< */,
> +                                        uint16_t                               nKTLevels  /**< */,

Sigh.

Okay, I'd rather not change the libxcb API. I'll respin the patch
without touching <sumof>.

Thanks for the review.

Peter Harris
On 2014-04-29 11:39, Ran Benita wrote:
> Hi Peter
> 
> On Mon, Apr 28, 2014 at 05:05:10PM -0400, Peter Harris wrote:
>> nLevelsPerType is nLevels long, and ktLevelNames is nKTLevels long.
> 
> Can you split the nKTLevels -> nTypes changes to a different commit?
> They obviously seem correct to me, so for those:
> Reviewed-by: Ran Benita <ran234@gmail.com>
> Tested-by: Ran Benita <ran234@gmail.com>

Pushed. Thanks for the review.

Peter Harris
On Tue, Apr 29, 2014 at 11:51:56 -0400, Peter Harris wrote:

> > Also, unfortunately, they cause this diff:
> > 
> > diff --git a/src/xkb.h b/src/xkb.h
> > index 845d1c9..3820bb1 100644
> > --- a/src/xkb.h
> > +++ b/src/xkb.h
> > @@ -9371,6 +9371,7 @@ xcb_xkb_get_names_value_list_radio_group_names_end (const xcb_xkb_get_names_repl
> >  int
> >  xcb_xkb_get_names_value_list_serialize (void                                 **_buffer  /**< */,
> >                                          uint8_t                                nTypes  /**< */,
> > +                                        uint16_t                               nKTLevels  /**< */,
> 
> Sigh.
> 
> Okay, I'd rather not change the libxcb API. I'll respin the patch
> without touching <sumof>.
> 
That's libxcb-xkb, not libxcb.  Maybe that's not as bad.

Cheers,
Julien