Merge lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options into lp:compiz/0.9.10
Status: | Merged |
---|---|
Approved by: | Sam Spilsbury |
Approved revision: | 3662 |
Merged at revision: | 3667 |
Proposed branch: | lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options |
Merge into: | lp:compiz/0.9.10 |
Diff against target: |
300 lines (+93/-43) 3 files modified
plugins/screenshot/screenshot.xml.in (+30/-5) plugins/screenshot/src/screenshot.cpp (+62/-38) plugins/screenshot/src/screenshot.h (+1/-0) |
To merge this branch: | bzr merge lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Approve | |
Sam Spilsbury | Approve | ||
MC Return | Needs Resubmitting | ||
Review via email: mp+159038@code.launchpad.net |
Commit message
*Screenshot, xml:
Better plugin summary.
Added option to disable the drawing of the screenshot
selection indicator.
Added options to change color and opacity of the outline
and the inside of the rectangle indicator, default colors
have not been changed.
Other minor improvements.
*Screenshot, code:
Introduced the new variable bool selectionSizeCh
which is true if the size of the selection has changed.
We just want to draw the screenshot selection box, if
we are grabbed, the size has changed and the CCSM
option to draw it is enabled.
Do not render anything if indicator is disabled.
Indicator colors are now configurable.
Increased indicator box outline line width to 2.0.
Only damage the full screen once, if the grab gets
terminated, during grab just damage the region painted,
which is the screenshot selection box (LP: #1172234).
*SCreenshot, cleanup:
Minor cleanup in the rendering code.
Removed redundant #ifndef USE_GLES compiler option,
GLES can cope with enabling and disabling GL_BLEND.
Use prefix- instead of postfix increments.
Declaration and assignment of local variables in one line.
Reduced the scope of the variables x1, y1, x2 and y2.
Replaced magic number 65535.0f with const float MaxUShortFloat =
std::numeric_limits <unsigned short>::max ();.
Indentation fixes.
C++ style for comments, added comments.
Looks mostly good, just a few tips:
115 +#ifndef USE_GLES
116 + glEnable (GL_BLEND);
117 +#endif
147 -#ifndef USE_GLES
148 - glEnable (GL_BLEND);
149 -#endif
Its not necessary to move that as it only needs to be enabled when draw commands are sent to the GPU. So it can stay in the same place.
119 + float alpha = ((float) optionGetFillCo lorAlpha () / 65535.0f);
This can be replaced with
float alpha = optionGetFillCo lorAlpha () / static_cast <float> (std::numeric_ limits <unsigned short>::max ());
Three things to note with that:
1. In order to get floating point division, you only need to have the denominator be a float and not the numerator.
2. std::numeric_limits <unsigned short::max () is the preferable way of saying 65536
3. static_cast should be preferred over c-style casts (eg (Type));
also:
125 + colorData[3] = alpha * 65535.0f;
becomes
colorData[3] = alpha * std::numeric_limits <unsigned short>::max ();
Finally, there are a few formatting changes scattered around here (though not as much as before). Try to split those two up as much as possible, as the areas where formatting changes were made were not really related to what this change is doing. They're fine to keep though, as they're correct.