[CLOSED] [Initial review] Update rule label when selector is edited

[core-ai-bot]

Issue by njx
Thursday Oct 17, 2013 at 04:04 GMT
Originally opened as adobe/brackets#5541

---

@dangoor - note that this is branched off the css-quick-edit-new-selector branch from #5532 (I didn't want to disturb your review there). The commits in this one (607b35d, df1d141, 73e7414) are semantic so can be reviewed individually.

---

njx included the following code: https://github.com/adobe/brackets/pull/5541/commits

[core-ai-bot]

Comment by njx
Thursday Oct 17, 2013 at 04:07 GMT

---

@peterflynn - might want to do a drive-by on df1d141

[core-ai-bot]

Comment by njx
Thursday Oct 17, 2013 at 04:11 GMT

---

One thing that's a little skeezy about this is that the way we update the selector when you make an edit is different from how we collect the selectors originally. This is mainly because of the mismatch between what findMatchingRules does (which searches by individual selector) and what we want here, which is to get all the selectors for a rule. I felt that doing it a different way here was fine since this aspect of CSS syntax is pretty simple.

[core-ai-bot]

Comment by njx
Thursday Oct 17, 2013 at 04:35 GMT

---

Hmmm...thinking about this again, I suppose this is a little more skeezy than I thought. For example, there could be a comment between the selector list and the beginning of the rule, and we would end up sucking that in. Doesn't seem super-likely, but it could happen...

It also occurs to me that another edge case would be if you had multiple rules on the same line--since TextRange only tracks line numbers, not offsets within a line, we would grab the first selector on the line even if the rule were later on the same line. Again, doesn't seem super-likely (aside from minified files, which would be a mess in an inline editor anyway)...but adding that level of tracking to TextRange is definitely more work (though perhaps not terrible).

Comments welcome...

[core-ai-bot]

Comment by dangoor
Thursday Oct 17, 2013 at 15:44 GMT

---

The code all looks fine for what I does.

It would be good, I think, if the mechanism used for getting the updated selector was the same. I tried out the edge cases you pointed out, and they are a bit ugly. That said, these are edge cases and I don't know how likely it is that someone would hit them in real life. Do people really put multiple rules on the same line in source files?

If it was quick and easy to change this, then I think we're better off fixing these edge cases now. If not, I think we should wait and see if anyone complains because it's entirely possible that we'd never hear a complaint about this.

[core-ai-bot]

Comment by njx
Thursday Oct 17, 2013 at 17:36 GMT

---

I think the only case for multiple lines on the same file is in a minified file. Other than that, I've never seen it.

It seems like there are basically two separate things we would want to fix:

(1) Use the real tokenizer to get the updated selector. I just realized that I could probably use extractAllSelectors() for this now since I just made it track selector groups. So I'll fix that.
(2) Make sure to get the right rule if there are multiple rules on the same line by extending TextRange to handle character offsets. I think this is the one that is lower bang-for-the-buck, since it's really unlikely that people will hit it. I'll see how much work it is, though--if it's not that bad I might as well just do it. (Seems like it might be useful in other cases too.)

[core-ai-bot]

Comment by dangoor
Thursday Oct 17, 2013 at 17:50 GMT

---

That sounds like a good plan.

[core-ai-bot]

Comment by njx
Thursday Oct 17, 2013 at 20:17 GMT

---

@dangoor - I pushed up the change to make getRangeSelectors() use the existing extractAllSelectors() code. I decided not to go for the TextRange change right now since it would be fairly intricate and doesn't seem worth the effort given the unlikelihood of the use case. So I think this is ready for review.

[core-ai-bot]

Comment by dangoor
Thursday Oct 17, 2013 at 20:46 GMT

---

Looks good. Merging.