Plasma::Svg: Do not require exact match for size hinted elements.

Review Request #5689 - Created Oct. 24, 2010 and submitted

Information
Ingomar Wesp
kde
Reviewers
plasma
Previously, if an SVG contained size hinted elements, they were only used when the display size matched the size hint exactly. This patch tries to relax this condition by searching for the smallest size hinted element that is still bigger than the display size (in order for the element to be chosen, it also has to have the same aspect ratio). If no such element can be found, it falls back to the normal element id as passed.

In order to speed up the lookup (and because it appears to be impossible to access the DOM of an already loaded SvgRenderer), all size hinted element ids are stored in SharedSvgRenderer at load time.

I think it would be good to change the QRegExp based id fetching into a proper DOM traversal. Are there any convenience functions in KDELibs that allow easy iterating over all elements (couldn't find any) or do I have to implement that myself based on Qt's DOM classes?

Please tell me what you think... Have I missed something?

  
Ingomar Wesp
Manuel Mommertz
Ingomar Wesp
Manuel Mommertz
Marco Martin
Ingomar Wesp
Ingomar Wesp
Ingomar Wesp
Marco Martin
Ingomar Wesp
Review request changed

Change Summary:

As suggested by Manuel, this adds a macro for natural size hint cache keys, uses a non-greedy regexp to find size hinted elements and makes the theme return just a list of rect cache keys instead of a full map between keys and values.

Additionally, the implicit QByteArray->QString conversion in QRegExp::indexIn(...) has been pulled out of the loop by explicitly converting the QByteArray beforehand.

Diff:

Revision 7 (+169 -13)

Show changes

Manuel Mommertz

   
/trunk/KDE/kdelibs/plasma/svg.cpp (Diff revision 7)
 
 
should end with:
-(.*)\\1");
this is save as setMinimal is true;
  1. Actually, I think it's a bit cleaner if we only accept size hinted elements whose base id is non-empty - (\\d+)-(\\d+)-(.*) would also accept "32-32-" for example. Although it does not matter in practice, because findInCache checks whether the passed element is non-empty anyways.
  2. I prefer wrong positives that are actually valid id's. Say we have a id "32-32-". With (.*)\\1 we get an empty match. But with (.+)\\1 the quotation mark gets part of .+ so we get all things up to the next quotation mark as id. And if the next quotation is the start of the next id (extreme unlikely but possible) we miss it completly.
/trunk/KDE/kdelibs/plasma/svg.cpp (Diff revision 7)
 
 
while (...) {
(coding style)
/trunk/KDE/kdelibs/plasma/svg.cpp (Diff revision 7)
 
 
while (...) {
(coding style)
/trunk/KDE/kdelibs/plasma/svg.cpp (Diff revision 7)
 
 
if (elementRect.isValid()) actualTheme()->...
/trunk/KDE/kdelibs/plasma/theme.cpp (Diff revision 7)
 
 
QString &key = i.next();
/trunk/KDE/kdelibs/plasma/theme.cpp (Diff revision 7)
 
 
key.resize(key.size() - 4)
(should be faster)
Manuel Mommertz
Somehow my comment on the last was lost... so again:

Fix the minor things marked above and I am fine with this. You did a good Job and it was fun to work with you. :)
Loading...