GNU bug report logs -
#22923
[PATCH] Support completion of attribute values in CSS mode
Previous Next
Reported by: Simen Heggestøyl <simenheg <at> gmail.com>
Date: Sun, 6 Mar 2016 13:55:02 UTC
Severity: wishlist
Tags: patch
Done: Simen Heggestøyl <simenheg <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 22923 in the body.
You can then email your comments to 22923 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Sun, 06 Mar 2016 13:55:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Simen Heggestøyl <simenheg <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Sun, 06 Mar 2016 13:55:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hello!
I've finally gotten around to implement property value completion in
CSS mode. I've been using the attached patch privately for some weeks
now, and it seems to me that it works well.
The code has taken much inspiration from company-css.el, but there are
some differences:
- Completion lists for many new properties have been added, and a few
have been removed (most of them from the obsoleted marquee
module [1]).
- I've manually updated all completion lists according to the CSS spec
and fixed some bugs in the existing lists along the way. In general
I've tried to stay as close as possible to the grammar described in
the CSS spec wrt. names and order of the values and classes.
- The new `css--property-values' function is very similar to
`company-css-property-values', but it has been updated to support
following completion candidates further from the value class
completion lists.
- Completion a function name will no longer put commas between the
parenthesis. My reason for this is that many CSS functions take a
variable number of arguments. For instance, the `translate' function
is completed by company-css to `translate(,)', but `translate' can
take only one argument too, so the completion is misleading. Also,
there are functions that support an arbitrary number of
arguments. Instead of trying to support this, I've made every
function name complete to just `function()'. I think having ElDoc
support for CSS functions would be good for making it easier to
remember which arguments functions take.
- `inherit' has been added as a completion candidate for every
property.
Please have a look at the patch and tell me what you think!
-- Simen
[1] https://www.w3.org/TR/css3-marquee/
[Message part 2 (text/html, inline)]
[0001-Support-completion-of-attribute-values-in-CSS-mode.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Sun, 06 Mar 2016 19:11:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 22923 <at> debbugs.gnu.org (full text, mbox):
Hi Simen,
On 03/06/2016 03:53 PM, Simen Heggestøyl wrote:
> I've finally gotten around to implement property value completion in
> CSS mode. I've been using the attached patch privately for some weeks
> now, and it seems to me that it works well.
I haven't tried it, but it looks good overall.
> - The new `css--property-values' function is very similar to
> `company-css-property-values', but it has been updated to support
> following completion candidates further from the value class
> completion lists.
Doesn't the latter allow for several indirections? It seems to me the
two functions are equivalent.
One question: is there a reason to do the (symbol-name value) conversion
before doing the css--property-values lookup?
Maybe the css-value-class-alist should have symbol keys, not strings.
> - Completion a function name will no longer put commas between the
> parenthesis. My reason for this is that many CSS functions take a
> variable number of arguments. For instance, the `translate' function
> is completed by company-css to `translate(,)', but `translate' can
> take only one argument too, so the completion is misleading. Also,
> there are functions that support an arbitrary number of
> arguments. Instead of trying to support this, I've made every
> function name complete to just `function()'. I think having ElDoc
> support for CSS functions would be good for making it easier to
> remember which arguments functions take.
Sounds good.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Wed, 09 Mar 2016 19:03:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 22923 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi Dmitry, thanks for your review.
On Sun, Mar 6, 2016 at 8:10 PM, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> Doesn't the latter allow for several indirections? It seems to me the
> two functions are equivalent.
It does, sorry for being unclear. I'll try to reformulate it:
There are two lists that must be traversed to find completion
candidates: the property alist and the value class alist. company-css.el
supports indirections from the property alist into itself, and into the
value class alist. With this patch, indirection can happen from the
property alist into itself and into the value class alist, but also from
the value class alist into itself and back into the property alist.
> One question: is there a reason to do the (symbol-name value)
> conversion before doing the css--property-values lookup?
>
> Maybe the css-value-class-alist should have symbol keys, not strings.
With the change mentioned above, the property alist and the value class
alist are now treated like the same kind of data structure by
`css--property-values'. The only difference is that the CARs of the
entries in the property alist are also valid property names. So I
thought it simpler if the keys were of the same data type. I don't have
an opinion on whether that should be string or symbol, just that they
are the same.
Did this answer your question?
-- Simen
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Fri, 11 Mar 2016 01:26:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 22923 <at> debbugs.gnu.org (full text, mbox):
On 03/09/2016 09:01 PM, Simen Heggestøyl wrote:
> There are two lists that must be traversed to find completion
> candidates: the property alist and the value class alist. company-css.el
> supports indirections from the property alist into itself, and into the
> value class alist. With this patch, indirection can happen from the
> property alist into itself and into the value class alist, but also from
> the value class alist into itself and back into the property alist.
I see. Is there anything in particular this approach is buying us? Any
properties that company-css doesn't support now, and would be suboptimal
using its current approach?
IMHO, having the indirection go only one way is better organization, and
it's easier to understand when reading the code.
> With the change mentioned above, the property alist and the value class
> alist are now treated like the same kind of data structure by
> `css--property-values'. The only difference is that the CARs of the
> entries in the property alist are also valid property names. So I
> thought it simpler if the keys were of the same data type. I don't have
> an opinion on whether that should be string or symbol, just that they
> are the same.
>
> Did this answer your question?
Yes, thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Sat, 19 Mar 2016 12:43:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 22923 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hello again, Dmitry, and sorry for the late response.
On Fri, Mar 11, 2016 at 2:25 AM, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> I see. Is there anything in particular this approach is buying us?
> Any properties that company-css doesn't support now, and would be
> suboptimal using its current approach?
Yes, it allows us to stay close to the CSS spec, which is my view is
very valuable when maintaining these lists.
Here is a concrete example: the value class `image' is defined as
follows in the CSS Image Values spec [1]:
<image> = <url> | <image-list> | <element-reference> | <gradient>
Which translates naturally to:
("image" uri image-list element-reference gradient)
It is not a CSS property, so it should go into the value class alist. It
is referenced by the `border-image-source' property as well as the
`bg-image' value class (which in turn is referenced by the
`background-image' property and `bg-layer' value class).
My point is that even though it would be possible to eliminate the need
for this value class by expanding it where it is referenced, I think
that by keeping it, it'll be much easier to make updates to it when the
CSS spec changes. I think it is worth the added complexity.
-- Simen
[1] https://www.w3.org/TR/css3-images/
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Sun, 20 Mar 2016 01:18:01 GMT)
Full text and
rfc822 format available.
Message #20 received at 22923 <at> debbugs.gnu.org (full text, mbox):
Hi again, Simen,
On 03/19/2016 02:42 PM, Simen Heggestøyl wrote:
> Yes, it allows us to stay close to the CSS spec, which is my view is
> very valuable when maintaining these lists.
Sure, but by "current approach" I meant what Company does. Please clarify:
> Here is a concrete example: the value class `image' is defined as
> follows in the CSS Image Values spec [1]:
>
> <image> = <url> | <image-list> | <element-reference> | <gradient>
>
> Which translates naturally to:
>
> ("image" uri image-list element-reference gradient)
>
> It is not a CSS property, so it should go into the value class alist. It
> is referenced by the `border-image-source' property as well as the
> `bg-image' value class (which in turn is referenced by the
> `background-image' property and `bg-layer' value class).
If you were adding it to company-css, wouldn't you just add it to
company-css-value-classes? And then refer to it in background-image
value inside company-css-property-alist?
What the limitation of that approach? Do value classes in the spec refer
back to the actual properties sometimes?
> My point is that even though it would be possible to eliminate the need
> for this value class by expanding it where it is referenced, I think
> that by keeping it, it'll be much easier to make updates to it when the
> CSS spec changes. I think it is worth the added complexity.
I'm not sure I follow. Expanding it in company-css-property-alist?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Mon, 21 Mar 2016 09:15:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 22923 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Sun, Mar 20, 2016 at 2:17 AM, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> If you were adding it to company-css, wouldn't you just add it to
> company-css-value-classes? And then refer to it in background-image
> value inside company-css-property-alist?
>
> What the limitation of that approach? Do value classes in the spec
> refer back to the actual properties sometimes?
Yes, though on closer inspection, I see that it only happens in one case
so far (the value class `bg-layer' referring to the property
`background-color').
But more importantly, in the example above, the `image' value class
refers to four other value classes, one of which has further references.
This case is not handled by the current company-css, since it only
recurses on values from the property alist.
This is the main difference between `company-css-property-values' and
`css--property-values': the former doesn't support referencing from the
value class alist.
(Indeed, there seems to be a bug in `company-css-value-classes' where
`align-stretch' tries to refer to `align-common', but that won't work.)
> I'm not sure I follow. Expanding it in company-css-property-alist?
I meant that if we wanted to keep the value class alist flat, i.e. don't
put references in it, we could replace every entry with the result of
`(company-css-property-values "value-class-name")'.
-- Simen
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Mon, 21 Mar 2016 11:03:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 22923 <at> debbugs.gnu.org (full text, mbox):
On 03/21/2016 11:14 AM, Simen Heggestøyl wrote:
> Yes, though on closer inspection, I see that it only happens in one case
> so far (the value class `bg-layer' referring to the property
> `background-color').
It seems like you could replace that reference with `color'.
> But more importantly, in the example above, the `image' value class
> refers to four other value classes, one of which has further references.
> This case is not handled by the current company-css, since it only
> recurses on values from the property alist.
>
> This is the main difference between `company-css-property-values' and
> `css--property-values': the former doesn't support referencing from the
> value class alist.
>
> (Indeed, there seems to be a bug in `company-css-value-classes' where
> `align-stretch' tries to refer to `align-common', but that won't work.)
You're right, it's a bug. I'd fix that with only making
company-css-value-classes recursive (but not refer back to
company-css-property-alist there).
To put it differently, I don't like that there's conflation of property
values and property names: if there appears a value sometimes that is
the same as some property's name (unlikely, I know), it would be hard to
represent in the proposed structure.
Anyway, I've taken more than enough of your time. Please go ahead with
whichever version you prefer.
Reply sent
to
Simen Heggestøyl <simenheg <at> gmail.com>:
You have taken responsibility.
(Wed, 23 Mar 2016 18:19:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Simen Heggestøyl <simenheg <at> gmail.com>:
bug acknowledged by developer.
(Wed, 23 Mar 2016 18:19:03 GMT)
Full text and
rfc822 format available.
Message #31 received at 22923-done <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Mon, Mar 21, 2016 at 12:02 PM, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> You're right, it's a bug. I'd fix that with only making
> company-css-value-classes recursive (but not refer back to
> company-css-property-alist there).
>
> To put it differently, I don't like that there's conflation of
> property values and property names: if there appears a value
> sometimes that is the same as some property's name (unlikely, I
> know), it would be hard to represent in the proposed structure.
>
> Anyway, I've taken more than enough of your time. Please go ahead
> with whichever version you prefer.
Don't worry Dmitry, I very much appreciate your through reviews.
You are right that there are some cases where property names and value
class names are the same (there are currently four), and I found a bug
in my handling of one of those. I remedied that by going with your
suggestion of making value classes symbols again, and only referring to
other value classes from those.
For properties, I made them also refer to value classes by default, but
made it possible to refer back to other properties in the cases where
the reference isn't found in the value class alist.
I attempted to document this thoroughly in the code and added some tests
that should cover the tricky cases.
I've installed the patch with the changes mentioned above.
Thanks again!
-- Simen
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#22923; Package
emacs.
(Wed, 23 Mar 2016 23:09:01 GMT)
Full text and
rfc822 format available.
Message #34 received at 22923 <at> debbugs.gnu.org (full text, mbox):
On 03/23/2016 08:17 PM, Simen Heggestøyl wrote:
> You are right that there are some cases where property names and value
> class names are the same (there are currently four), and I found a bug
> in my handling of one of those. I remedied that by going with your
> suggestion of making value classes symbols again, and only referring to
> other value classes from those.
>
> For properties, I made them also refer to value classes by default, but
> made it possible to refer back to other properties in the cases where
> the reference isn't found in the value class alist.
>
> I attempted to document this thoroughly in the code and added some tests
> that should cover the tricky cases.
>
> I've installed the patch with the changes mentioned above.
Great! Thanks, Simen.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 21 Apr 2016 11:24:03 GMT)
Full text and
rfc822 format available.
This bug report was last modified 9 years and 121 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.