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

quick hover #27

Merged
merged 4 commits into from
Jul 8, 2015
Merged

quick hover #27

merged 4 commits into from
Jul 8, 2015

Conversation

hrj
Copy link
Contributor

@hrj hrj commented Jul 7, 2015

For #17

API to get selectors that match a pseudo clause

I have created a new class called AnalyserUtil which is designed to be pure (state-less) class. The functions in this class are all declared static. There are only 4 public functions: getApplicableRules, getElementStyle, hasPseudoSelctor, hasPseudoSelectorForAncestor. The idea is that clients can use these functions efficiently, without necessarily storing / recreating an Analyser instance. They can choose to cache the intermediate results (such as the result of getApplicableRules).

This has been done without modifying the existing APIs and verifying that all unit tests pass. I have also tested everything in gngr.

Minor cleanup

  • Fixed javadoc errors
  • Simplified the function: SelectorImpl.getPseudoElement
  • Updated version of unbescape to latest, 1.1.1

hrj added 4 commits July 7, 2015 16:43
* Refactored some of the previous implementations into a pure static
class, keeping the existing API intact.

s* mall optimisation: use array instead of array list
radkovo added a commit that referenced this pull request Jul 8, 2015
New API for checking pseudo selectors (for quick hover) + analyzer refactoring
@radkovo radkovo merged commit 4777ad3 into radkovo:master Jul 8, 2015
@radkovo
Copy link
Owner

radkovo commented Jul 8, 2015

Great, thanks for the refactoring. I like it very much. Merged.

@hrj
Copy link
Contributor Author

hrj commented Jul 8, 2015

Thanks for merging. There is a small bug that I just discovered now. I will send a PR soon.

Also, there is a possibility for optimisation. When finding applicable rules, all selectors get added. For example, when finding applicable selectors for a p element, if the CSS is:

div, p {
  color: red
}

then this rules gets added as it is. That is both, div and p selectors are retained.

It would be a nice optimisation if only the p selector was taken. I am trying to do this, but got stuck a little bit. Here's my working code:

final List<OrderedRule> nameRules = holder.get(HolderItem.ELEMENT, nameLC);
if (nameRules != null) {
    // Trying not to add all of them
    // candidates.addAll(nameRules);

    for (OrderedRule nr: nameRules) {
        ArrayList<CombinedSelector> matchingRules = new ArrayList<CombinedSelector>();
        for (CombinedSelector cs: nr.getRule().getSelectors()) {
            if (cs.getLastSelector().getElementName().equals(nameLC)) {
                matchingRules.add(cs);
            }
        }
        final int matchCount = matchingRules.size();
        RuleSetImpl ruleSet = new RuleSetImpl(matchingRules.toArray(new CombinedSelector[matchCount]));
        ruleSet.addAll(nr.getRule());
        candidates.add(new OrderedRule(ruleSet, nr.getOrder()));
    }
}

However, the line ruleSet.addAll() throws a runtime exception, because it is an abstract list. It looks like we need to create a rule factory for this, but that sounds like a big overhead. Do you have any ideas for this?

Thanks.

@hrj
Copy link
Contributor Author

hrj commented Jul 8, 2015

Btw, the bug is in the hasPseudoDeclaratorForAncestor() function. So you don't need to worry; It doesn't affect existing functionality.

@hrj
Copy link
Contributor Author

hrj commented Jul 8, 2015

Good news: There is no bug in jStyleParser code. I was using it slightly wrong in the browser. (When the mouse hovers out of an element, I was changing the match condition and then calling hasPseudoDeclaratorForAncestor, which was causing the match to fail. I am now calling it before changing the match condition and it works fine.)

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

Successfully merging this pull request may close these issues.

2 participants