Re: Review Request: Make mouse cursor size configurable
On Nov. 9, 2011, 6:48 a.m., Fredrik Höglund wrote: kcontrol/input/xcursor/cursortheme.cpp, line 119 http://git.reviewboard.kde.org/r/101701/diff/2/?file=33417#file33417line119 Scaling a pixmap is more expensive than scaling an image. Internally it involves converting the pixmap back to an image, scaling it, and then converting the image back to a pixmap. This is not a major issue, but it's the reason the image was scaled before it was converted to a pixmap. Lukas Sommer wrote: The problem is that I can't drop createIcon(int size) because I need it on other places. So I would either have to copy the code to createIcon() or change the return type of createIcon to QImage. Maybe we can leave this for a later revision? Fredrik Höglund wrote: Like I said, it's not an issue that needs to be fixed now. A patch is available as review request #104077 but as there, it was commented that this is no longer necessary with Qt 4.8, I have discarted the review request. If this is not okay, let me know. - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review8036 --- On Sept. 2, 2011, 4:40 p.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated Sept. 2, 2011, 4:40 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Description --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/cursortheme.h 586ccba kcontrol/input/xcursor/cursortheme.cpp 92abea5 kcontrol/input/xcursor/legacytheme.h 846bf9b kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/themepage.h 38ca893 kcontrol/input/xcursor/themepage.cpp 6c9f29a kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/xcursortheme.cpp 2ecb9ba Diff: http://git.reviewboard.kde.org/r/101701/diff/ Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/248/ Thanks, Lukas Sommer
Re: Review Request: kcontrol mouse coursor: better performance
On Feb. 26, 2012, 7:12 p.m., Christoph Feck wrote: With Qt 4.8, there is usually no difference between QImage and QPixmap, so I do not think we should try to be smart here. There general rule is: Use QImage, if you need to access individual pixels, use QPixmap everywhere else. This way we can benefit from any optimizations that can only be applied to opaque datatypes (which QImage isn't). Okay. It was just because Fredrik Höglund commented in #101701 that I should implement this with a later patch. However, if this isn't necessary with Qt 4.8 anymore, I discard this review request. - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104077/#review10918 --- On Feb. 25, 2012, 5:42 p.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104077/ --- (Updated Feb. 25, 2012, 5:42 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, Fredrik Höglund, and Christoph Feck. Description --- With review request r101701 being submitted, there is still a remaining issue that wasn't solved: Scaling a QImage is expensive. So it is better to keep the data as QPixmap as long as necessary. This patch implements a less expensive behaviour. Diffs - kcontrol/input/xcursor/cursortheme.cpp 2c8c260 kcontrol/input/xcursor/themepage.cpp 0f678ed Diff: http://git.reviewboard.kde.org/r/104077/diff/ Testing --- Thanks, Lukas Sommer
Review Request: kcontrol mouse coursor: better performance
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104077/ --- Review request for KDE Base Apps, KDE Runtime, kdelibs, Fredrik Höglund, and Christoph Feck. Description --- With review request r101701 being submitted, there is still a remaining issue that wasn't solved: Scaling a QImage is expensive. So it is better to keep the data as QPixmap as long as necessary. This patch implements a less expensive behaviour. Diffs - kcontrol/input/xcursor/cursortheme.cpp 2c8c260 kcontrol/input/xcursor/themepage.cpp 0f678ed Diff: http://git.reviewboard.kde.org/r/104077/diff/ Testing --- Thanks, Lukas Sommer
Re: Review Request: Make mouse cursor size configurable
On Nov. 9, 2011, 6:48 a.m., Fredrik Höglund wrote: kcontrol/input/xcursor/cursortheme.cpp, line 119 http://git.reviewboard.kde.org/r/101701/diff/2/?file=33417#file33417line119 Scaling a pixmap is more expensive than scaling an image. Internally it involves converting the pixmap back to an image, scaling it, and then converting the image back to a pixmap. This is not a major issue, but it's the reason the image was scaled before it was converted to a pixmap. The problem is that I can't drop createIcon(int size) because I need it on other places. So I would either have to copy the code to createIcon() or change the return type of createIcon to QImage. Maybe we can leave this for a later revision? On Nov. 9, 2011, 6:48 a.m., Fredrik Höglund wrote: kcontrol/input/xcursor/cursortheme.h, line 80 http://git.reviewboard.kde.org/r/101701/diff/2/?file=33416#file33416line80 I'm having trouble working out if changing the default from -1 to 0 is an unrelated change or not. The implementation now treats not only -1 as default value (=resolution dependend) but every value = 0. This is important because 0 also means resolutin dependend. I think it would make no difference to leave the header as is, but I changed the header because this makes clear that 0 is not a valid size. - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review8036 --- On Sept. 2, 2011, 4:40 p.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated Sept. 2, 2011, 4:40 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Description --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/cursortheme.h 586ccba kcontrol/input/xcursor/cursortheme.cpp 92abea5 kcontrol/input/xcursor/legacytheme.h 846bf9b kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/themepage.h 38ca893 kcontrol/input/xcursor/themepage.cpp 6c9f29a kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/xcursortheme.cpp 2ecb9ba Diff: http://git.reviewboard.kde.org/r/101701/diff/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/248/ Thanks, Lukas Sommer
Re: Review Request: Make mouse cursor size configurable
On Nov. 9, 2011, 6:48 a.m., Fredrik Höglund wrote: Sorry for not finding the time to follow up on this until now. The new design is not quite what I had in mind, but given that the freeze is tomorrow I'm fine with including it in 4.8. I have given the code a quick look over, and I don't see any show stoppers, although I have some nitpicks below. You should also add yourself to the list of copyrights. When I come home from university in 9 hours, I will do this. I hope I'll be in time... What is about https://git.reviewboard.kde.org/r/102524/, can we ship this simultanly? - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review8036 --- On Sept. 2, 2011, 4:40 p.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated Sept. 2, 2011, 4:40 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Description --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/cursortheme.h 586ccba kcontrol/input/xcursor/cursortheme.cpp 92abea5 kcontrol/input/xcursor/legacytheme.h 846bf9b kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/themepage.h 38ca893 kcontrol/input/xcursor/themepage.cpp 6c9f29a kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/xcursortheme.cpp 2ecb9ba Diff: http://git.reviewboard.kde.org/r/101701/diff/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/248/ Thanks, Lukas Sommer
Re: Review Request: Make mouse cursor size configurable
On Sept. 21, 2011, 1:05 p.m., Christoph Feck wrote: What I dislike is the position of the size combo box. Either it should be below the list, or, when above the list, share the space with the preview. Regarding the use DPI depended size, what we could do if we use a slider, is to have a small Revert/Default button next to the slider, like what the new Locale KCM does. Then we can have both the slider, and a way to use the automatic size. I really would like to see this in 4.8, so if this feature isn't on the feature plan yet, please add it, if you need more time for changes. Fredrik, further comments? Lukas Sommer wrote: Now it's on the feature list. About the position: Is it okay whan I move it to below the list? About the slider: And how do we distinghish between resolution dependend size and manual size, choosen be the slider in the UI? The problem is that we don't know the resolution dependend size. In xcursors, there is a way to get cursor _pixmaps_ in the default size (that is what we are doing do display the icon for the entry resolution dependend in the combobox), but xcursors doesn't provide a way to ask for the cursor size _value_ directly. Furthermore, the question is: What happens when the user changes the resolution later? The cursor size should adopt automatically - otherwise, this option would not make sense. But if it adopts automatically, IMHO this should be reflected an an own state in the UI. Lukas Sommer wrote: About the position: Maybe the screenshot is missleading: Above the size combobox, there are the buttons Get new theme (GHNS), Install new theme and Remove theme. This is also the reason why the size combobox has such a big width: It adopts to the width of the buttons above in the grid. Christoph Feck wrote: It was indeed confusing, I had to start the current version to see that the menu hides the three buttons. Considering that, the position as shown is probably fine. Regarding button next to the slider, I was suggesting a compromise between Fredrik's suggestion, and our intention to offer a default (DPI depended) size. I am fine with the combo box, but Fredrik is the module maintainer :) @Fredrik Do we need further changes in the design? - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review6697 --- On Sept. 2, 2011, 4:40 p.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated Sept. 2, 2011, 4:40 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Description --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/cursortheme.h 586ccba kcontrol/input/xcursor/cursortheme.cpp 92abea5 kcontrol/input/xcursor/legacytheme.h 846bf9b kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/themepage.h 38ca893 kcontrol/input/xcursor/themepage.cpp 6c9f29a kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/xcursortheme.cpp 2ecb9ba Diff: http://git.reviewboard.kde.org/r/101701/diff/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/248/ Thanks, Lukas Sommer
Re: Review Request: Make mouse cursor size configurable
On Sept. 21, 2011, 1:05 p.m., Christoph Feck wrote: What I dislike is the position of the size combo box. Either it should be below the list, or, when above the list, share the space with the preview. Regarding the use DPI depended size, what we could do if we use a slider, is to have a small Revert/Default button next to the slider, like what the new Locale KCM does. Then we can have both the slider, and a way to use the automatic size. I really would like to see this in 4.8, so if this feature isn't on the feature plan yet, please add it, if you need more time for changes. Fredrik, further comments? Lukas Sommer wrote: Now it's on the feature list. About the position: Is it okay whan I move it to below the list? About the slider: And how do we distinghish between resolution dependend size and manual size, choosen be the slider in the UI? The problem is that we don't know the resolution dependend size. In xcursors, there is a way to get cursor _pixmaps_ in the default size (that is what we are doing do display the icon for the entry resolution dependend in the combobox), but xcursors doesn't provide a way to ask for the cursor size _value_ directly. Furthermore, the question is: What happens when the user changes the resolution later? The cursor size should adopt automatically - otherwise, this option would not make sense. But if it adopts automatically, IMHO this should be reflected an an own state in the UI. About the position: Maybe the screenshot is missleading: Above the size combobox, there are the buttons Get new theme (GHNS), Install new theme and Remove theme. This is also the reason why the size combobox has such a big width: It adopts to the width of the buttons above in the grid. - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review6697 --- On Sept. 2, 2011, 4:40 p.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated Sept. 2, 2011, 4:40 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Description --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/cursortheme.h 586ccba kcontrol/input/xcursor/cursortheme.cpp 92abea5 kcontrol/input/xcursor/legacytheme.h 846bf9b kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/themepage.h 38ca893 kcontrol/input/xcursor/themepage.cpp 6c9f29a kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/xcursortheme.cpp 2ecb9ba Diff: http://git.reviewboard.kde.org/r/101701/diff/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/248/ Thanks, Lukas Sommer
Re: Review Request: Make mouse cursor size configurable
On Sept. 21, 2011, 1:05 p.m., Christoph Feck wrote: What I dislike is the position of the size combo box. Either it should be below the list, or, when above the list, share the space with the preview. Regarding the use DPI depended size, what we could do if we use a slider, is to have a small Revert/Default button next to the slider, like what the new Locale KCM does. Then we can have both the slider, and a way to use the automatic size. I really would like to see this in 4.8, so if this feature isn't on the feature plan yet, please add it, if you need more time for changes. Fredrik, further comments? Now it's on the feature list. About the position: Is it okay whan I move it to below the list? About the slider: And how do we distinghish between resolution dependend size and manual size, choosen be the slider in the UI? The problem is that we don't know the resolution dependend size. In xcursors, there is a way to get cursor _pixmaps_ in the default size (that is what we are doing do display the icon for the entry resolution dependend in the combobox), but xcursors doesn't provide a way to ask for the cursor size _value_ directly. Furthermore, the question is: What happens when the user changes the resolution later? The cursor size should adopt automatically - otherwise, this option would not make sense. But if it adopts automatically, IMHO this should be reflected an an own state in the UI. - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review6697 --- On Sept. 2, 2011, 4:40 p.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated Sept. 2, 2011, 4:40 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Summary --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/cursortheme.h 586ccba kcontrol/input/xcursor/cursortheme.cpp 92abea5 kcontrol/input/xcursor/legacytheme.h 846bf9b kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/themepage.h 38ca893 kcontrol/input/xcursor/themepage.cpp 6c9f29a kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/xcursortheme.cpp 2ecb9ba Diff: http://git.reviewboard.kde.org/r/101701/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/248/ Thanks, Lukas
Review Request: systemsettings/fonts: DPI setting is X11-only
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102527/ --- Review request for KDE Base Apps, KDE Runtime and kdewin. Summary --- In systemsettings/fonts, the DPI setting affects only X11. Therefor, this setting should only be enabled when compiling on X11 (and not for compiling on MS Windows or other non-X11 window systems). Diffs - kcontrol/fonts/fonts.h 7f1c2d0 kcontrol/fonts/fonts.cpp 5a1728d Diff: http://git.reviewboard.kde.org/r/102527/diff Testing --- Tested on X11, adding an #undef Q_WS_X11 for testing purpose. Thanks, Lukas
Re: Review Request: Make mouse cursor size configurable
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated Sept. 2, 2011, 4:40 p.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Changes --- New screenshot. Summary --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/cursortheme.h 586ccba kcontrol/input/xcursor/cursortheme.cpp 92abea5 kcontrol/input/xcursor/legacytheme.h 846bf9b kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/themepage.h 38ca893 kcontrol/input/xcursor/themepage.cpp 6c9f29a kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/xcursortheme.cpp 2ecb9ba Diff: http://git.reviewboard.kde.org/r/101701/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/248/ Thanks, Lukas
Re: Review Request: Make mouse cursor size configurable
On July 12, 2011, 8:56 a.m., Christoph Feck wrote:
kcontrol/input/xcursor/xcursortheme.cpp, line 73
http://git.reviewboard.kde.org/r/101701/diff/1/?file=24784#file24784line73
Do you mean the sizes are always 0, or do you mean the number of
entries in the list is 0 (never empty)?
Do we need some logic to prevent it saying Available sizes: when only
one size is available? But I am not sure if i18n can handle that.
Lukas Sommer wrote:
It means never empty.
This could be a solution to the plural problem:
// The translation is aware of plural forms. i18ncp uses the
first integer argument to
// distinguish plural forms. The first and the second argument
are QString. So we use
// sizeList.size() as third argument to provide proper support
for plural forms. This
// works, although it is never referenced with %3 in the string
itself. Although we
// provide english strings only for 1 item and for more than 1
item, but i18ncp
// will silently expand to as many plural forms as necesarry in
the target language.
m_description = i18ncp(
@info/plain This is the description of the cursor themes. The
first argument is the
original description that comes from the index.theme file.
The second argument is
the list of available sizes, which is never empty. This
string is localized with
support for plural forms: You can make different
singular/plural translations
depending on the number (!) of list items in the second
argument.,
%1 (Size: %2)/* only 1 item in sizeListString
*/
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeliststring).arg(sizeList.size());
I'll test this after holiday.
Chusslove Illich wrote:
i18ncp() should not be used for clever tricks. The plural string should
always be the direct pluralization of the singular string.
I think it would be just fine to leave it at the original version, with
...
Available sizes: ... even if there is only one size. Also no need to
mention in the context that list of sizes is never empty.
Otherwise, use two separate messages with ordinary if-selection.
Lukas Sommer wrote:
What's about merging both approaches?
int sizeListSize = sizeList.size();
QString sizeListString = QString::number(sizeList.takeFirst());
while (!sizeList.isEmpty())
{
sizeListString.append(, );
sizeListString.append(QString::number(sizeList.takeFirst()));
};
if (sizeList.size() == 1)
m_description = i18ncp(
@info/plain This is the description of the cursor
themes. The first argument is
the original description that comes from the
index.theme file. The second
argument is the size (in pixel). Example:
'OriginalDescription (Size: 24)',
%1 (Size: %2)).arg(m_description).arg(sizeListString);
else // sizeList.size() 1
/* The translation is aware of plural forms. i18ncp uses the
first integer argument to
distinguish plural forms. The first and the second
argument are QString. So we use
sizeList.size() as third argument to provide proper
support for plural forms. This
works, although it is never referenced with %3 in the
string itself. Although we
provide english strings only for 1 item and for more
than 1 item, but i18ncp
will silently expand to as many plural forms as necesarry
in the target language. */
m_description = i18ncp(
@info/plain This is the description of the cursor
themes. The first argument is
the original description that comes from the
index.theme file. The second
argument is the list of available sizes (in pixel).
This string is localized
with support for plural forms: You can make
different singular/plural
translations depending on the number (!) of list
items in the second
argument. Example: 'OriginalDescription (Available
sizes: 24, 36, 48)' has
the plural form for 3 because the list has 3 items.,
%1 (Available size: %2) /* only 1 item in
sizeListString - will never be used */
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeListString).arg(sizeListSize);
This way
Re: Review Request: Make mouse cursor size configurable
On July 20, 2011, 2:46 p.m., Fredrik Höglund wrote: Lucas, first of all I want to say thank you for working on this. In good UI design it is important to think about the problem the user wants to solve from the user's point of view, and not in terms of how the underlying implementation works. Unfortunately I feel that this feature as it is now falls short of that mark in a number of ways. You are essentially giving the user a list of pixel sizes to choose from and an input box to type it in. To choose a size they have to be able to picture in their mind how large these sizes are in relation to the monitor. They also have to understand that if the size they type in isn't available, the Xcursor library will pick the closest size that is. Exposing all these technical details of how the Xcursor library works behind the scenes just detracts from the problem of wanting larger or smaller cursors. In my opinion it would be much more intuitive to have a slider that snaps to the sizes that are available, and update the preview when the user moves the slider. If the theme only has one size, you would indicate this by disabling the slider. Another option is to load a version of the left_ptr cursor for each available size and display them in the right-hand size of the listview. The user would select a size by clicking the one they want. I also don't think it is useful to offer the user a way to go back to choosing a size automatically based on a heuristic. The user can't predict what size that will be, so it's akin to having an option to pick a random size. If they have at one point made an active choice, they can just as easily make another one. The KCM should perhaps go back to using the heuristic if the user clicks the Defaults button in the KCM, but not otherwise. Lucas, first of all I want to say thank you for working on this. Thanks for the feedback :-) Unfortunately I feel that this feature as it is now falls short of that mark in a number of ways. Okay, let's change this :-) In my opinion it would be much more intuitive to have a slider Gnome is also doing this (http://www.bbc.co.uk/accessibility/images/guides/mouse_pointer/computer/mouse_pointer_gnome_fig3.jpg). And it seems that Mac OS also does it this way (http://kofler.info/uploads/images/blog/mac-bedienungshilfen.png). Another option is to load a version of the left_ptr cursor for each available size and display them in the right-hand size of the listview. Do you mean display the different cursor sizes image above the button Remove theme? Personally, I would prefer this one more than the slider. Somehow more directly for the user. I also don't think it is useful to offer the user a way to go back to choosing a size automatically based on a heuristic. The user can't predict what size that will be, so it's akin to having an option to pick a random size. If they have at one point made an active choice, they can just as easily make another one. Hm, here I disagree. I would like to see KDE's UI being more resolution independent (see also bug 272266). This means that the user has a simple option that makes everything bigger. The fonts, for example, are yet depending on the font DPI value. And also for the Oxygen widget style, there are plans to make it scalable. I think that it is a very important feature that the cursor adopts automatically to the DPI. Actually, KDE is doing this, and I would not like to remove this behavior completely. What's about this? Remove the current combobox and spinbox and add a label Size: above the Remove theme button, and above this label a new combobox. This combobox has as first entry Automatically, and the following entries are the available sizes. Example: Automatically, 24 or Automatically, 24, 36, 48. Each entry has an icon with the left_prt cursor in the corresponding size. Would this be okay? Or should I use another widget than the combobox? Maybe a small listview? - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review4898 --- On June 20, 2011, 10:04 a.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated June 20, 2011, 10:04 a.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Summary --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add
Re: Review Request: Make mouse cursor size configurable
On July 12, 2011, 8:56 a.m., Christoph Feck wrote: kcontrol/input/xcursor/themepage.ui, line 78 http://git.reviewboard.kde.org/r/101701/diff/1/?file=24782#file24782line78 sizePolicyComboBox, there is no Police here ;) How should I name this? - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review4633 --- On June 20, 2011, 10:04 a.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated June 20, 2011, 10:04 a.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Summary --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/xcursortheme.cpp a987487 kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/themepage.h 902148f kcontrol/input/xcursor/themepage.cpp 24b9efe kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/cursortheme.h 8f7834b kcontrol/input/xcursor/legacytheme.h 23c9d5f Diff: http://git.reviewboard.kde.org/r/101701/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/188/ http://git.reviewboard.kde.org/r/101701/s/189/ Thanks, Lukas
Re: Review Request: Make mouse cursor size configurable
On July 12, 2011, 8:56 a.m., Christoph Feck wrote:
kcontrol/input/xcursor/xcursortheme.cpp, line 73
http://git.reviewboard.kde.org/r/101701/diff/1/?file=24784#file24784line73
Do you mean the sizes are always 0, or do you mean the number of
entries in the list is 0 (never empty)?
Do we need some logic to prevent it saying Available sizes: when only
one size is available? But I am not sure if i18n can handle that.
Lukas Sommer wrote:
It means never empty.
This could be a solution to the plural problem:
// The translation is aware of plural forms. i18ncp uses the
first integer argument to
// distinguish plural forms. The first and the second argument
are QString. So we use
// sizeList.size() as third argument to provide proper support
for plural forms. This
// works, although it is never referenced with %3 in the string
itself. Although we
// provide english strings only for 1 item and for more than 1
item, but i18ncp
// will silently expand to as many plural forms as necesarry in
the target language.
m_description = i18ncp(
@info/plain This is the description of the cursor themes. The
first argument is the
original description that comes from the index.theme file.
The second argument is
the list of available sizes, which is never empty. This
string is localized with
support for plural forms: You can make different
singular/plural translations
depending on the number (!) of list items in the second
argument.,
%1 (Size: %2)/* only 1 item in sizeListString
*/
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeliststring).arg(sizeList.size());
I'll test this after holiday.
Chusslove Illich wrote:
i18ncp() should not be used for clever tricks. The plural string should
always be the direct pluralization of the singular string.
I think it would be just fine to leave it at the original version, with
...
Available sizes: ... even if there is only one size. Also no need to
mention in the context that list of sizes is never empty.
Otherwise, use two separate messages with ordinary if-selection.
Lukas Sommer wrote:
What's about merging both approaches?
int sizeListSize = sizeList.size();
QString sizeListString = QString::number(sizeList.takeFirst());
while (!sizeList.isEmpty())
{
sizeListString.append(, );
sizeListString.append(QString::number(sizeList.takeFirst()));
};
if (sizeList.size() == 1)
m_description = i18ncp(
@info/plain This is the description of the cursor
themes. The first argument is
the original description that comes from the
index.theme file. The second
argument is the size (in pixel). Example:
'OriginalDescription (Size: 24)',
%1 (Size: %2)).arg(m_description).arg(sizeListString);
else // sizeList.size() 1
/* The translation is aware of plural forms. i18ncp uses the
first integer argument to
distinguish plural forms. The first and the second
argument are QString. So we use
sizeList.size() as third argument to provide proper
support for plural forms. This
works, although it is never referenced with %3 in the
string itself. Although we
provide english strings only for 1 item and for more
than 1 item, but i18ncp
will silently expand to as many plural forms as necesarry
in the target language. */
m_description = i18ncp(
@info/plain This is the description of the cursor
themes. The first argument is
the original description that comes from the
index.theme file. The second
argument is the list of available sizes (in pixel).
This string is localized
with support for plural forms: You can make
different singular/plural
translations depending on the number (!) of list
items in the second
argument. Example: 'OriginalDescription (Available
sizes: 24, 36, 48)' has
the plural form for 3 because the list has 3 items.,
%1 (Available size: %2) /* only 1 item in
sizeListString - will never be used */
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeListString).arg(sizeListSize);
This way
Re: Review Request: Make mouse cursor size configurable
On July 12, 2011, 8:56 a.m., Christoph Feck wrote:
kcontrol/input/xcursor/xcursortheme.cpp, line 73
http://git.reviewboard.kde.org/r/101701/diff/1/?file=24784#file24784line73
Do you mean the sizes are always 0, or do you mean the number of
entries in the list is 0 (never empty)?
Do we need some logic to prevent it saying Available sizes: when only
one size is available? But I am not sure if i18n can handle that.
Lukas Sommer wrote:
It means never empty.
This could be a solution to the plural problem:
// The translation is aware of plural forms. i18ncp uses the
first integer argument to
// distinguish plural forms. The first and the second argument
are QString. So we use
// sizeList.size() as third argument to provide proper support
for plural forms. This
// works, although it is never referenced with %3 in the string
itself. Although we
// provide english strings only for 1 item and for more than 1
item, but i18ncp
// will silently expand to as many plural forms as necesarry in
the target language.
m_description = i18ncp(
@info/plain This is the description of the cursor themes. The
first argument is the
original description that comes from the index.theme file.
The second argument is
the list of available sizes, which is never empty. This
string is localized with
support for plural forms: You can make different
singular/plural translations
depending on the number (!) of list items in the second
argument.,
%1 (Size: %2)/* only 1 item in sizeListString
*/
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeliststring).arg(sizeList.size());
I'll test this after holiday.
Chusslove Illich wrote:
i18ncp() should not be used for clever tricks. The plural string should
always be the direct pluralization of the singular string.
I think it would be just fine to leave it at the original version, with
...
Available sizes: ... even if there is only one size. Also no need to
mention in the context that list of sizes is never empty.
Otherwise, use two separate messages with ordinary if-selection.
Lukas Sommer wrote:
What's about merging both approaches?
int sizeListSize = sizeList.size();
QString sizeListString = QString::number(sizeList.takeFirst());
while (!sizeList.isEmpty())
{
sizeListString.append(, );
sizeListString.append(QString::number(sizeList.takeFirst()));
};
if (sizeList.size() == 1)
m_description = i18ncp(
@info/plain This is the description of the cursor
themes. The first argument is
the original description that comes from the
index.theme file. The second
argument is the size (in pixel). Example:
'OriginalDescription (Size: 24)',
%1 (Size: %2)).arg(m_description).arg(sizeListString);
else // sizeList.size() 1
/* The translation is aware of plural forms. i18ncp uses the
first integer argument to
distinguish plural forms. The first and the second
argument are QString. So we use
sizeList.size() as third argument to provide proper
support for plural forms. This
works, although it is never referenced with %3 in the
string itself. Although we
provide english strings only for 1 item and for more
than 1 item, but i18ncp
will silently expand to as many plural forms as necesarry
in the target language. */
m_description = i18ncp(
@info/plain This is the description of the cursor
themes. The first argument is
the original description that comes from the
index.theme file. The second
argument is the list of available sizes (in pixel).
This string is localized
with support for plural forms: You can make
different singular/plural
translations depending on the number (!) of list
items in the second
argument. Example: 'OriginalDescription (Available
sizes: 24, 36, 48)' has
the plural form for 3 because the list has 3 items.,
%1 (Available size: %2) /* only 1 item in
sizeListString - will never be used */
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeListString).arg(sizeListSize);
This way
Re: Review Request: Make mouse cursor size configurable
On July 12, 2011, 8:56 a.m., Christoph Feck wrote: kcontrol/input/xcursor/xcursortheme.cpp, line 73 http://git.reviewboard.kde.org/r/101701/diff/1/?file=24784#file24784line73 Do you mean the sizes are always 0, or do you mean the number of entries in the list is 0 (never empty)? Do we need some logic to prevent it saying Available sizes: when only one size is available? But I am not sure if i18n can handle that. It means never empty. This could be a solution to the plural problem: // The translation is aware of plural forms. i18ncp uses the first integer argument to // distinguish plural forms. The first and the second argument are QString. So we use // sizeList.size() as third argument to provide proper support for plural forms. This // works, although it is never referenced with %3 in the string itself. Although we // provide english strings only for 1 item and for more than 1 item, but i18ncp // will silently expand to as many plural forms as necesarry in the target language. m_description = i18ncp( @info/plain This is the description of the cursor themes. The first argument is the original description that comes from the index.theme file. The second argument is the list of available sizes, which is never empty. This string is localized with support for plural forms: You can make different singular/plural translations depending on the number (!) of list items in the second argument., %1 (Size: %2)/* only 1 item in sizeListString */ %1 (Available sizes: %2) /* more than 1 item in sizeListString */ ).arg(m_description).arg(sizeliststring).arg(sizeList.size()); I'll test this after holiday. - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/#review4633 --- On June 20, 2011, 10:04 a.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- (Updated June 20, 2011, 10:04 a.m.) Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Summary --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/xcursortheme.cpp a987487 kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/themepage.h 902148f kcontrol/input/xcursor/themepage.cpp 24b9efe kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/cursortheme.h 8f7834b kcontrol/input/xcursor/legacytheme.h 23c9d5f Diff: http://git.reviewboard.kde.org/r/101701/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/188/ http://git.reviewboard.kde.org/r/101701/s/189/ Thanks, Lukas
Re: Review Request: Make mouse cursor size configurable
On July 12, 2011, 8:56 a.m., Christoph Feck wrote:
kcontrol/input/xcursor/xcursortheme.cpp, line 73
http://git.reviewboard.kde.org/r/101701/diff/1/?file=24784#file24784line73
Do you mean the sizes are always 0, or do you mean the number of
entries in the list is 0 (never empty)?
Do we need some logic to prevent it saying Available sizes: when only
one size is available? But I am not sure if i18n can handle that.
Lukas Sommer wrote:
It means never empty.
This could be a solution to the plural problem:
// The translation is aware of plural forms. i18ncp uses the
first integer argument to
// distinguish plural forms. The first and the second argument
are QString. So we use
// sizeList.size() as third argument to provide proper support
for plural forms. This
// works, although it is never referenced with %3 in the string
itself. Although we
// provide english strings only for 1 item and for more than 1
item, but i18ncp
// will silently expand to as many plural forms as necesarry in
the target language.
m_description = i18ncp(
@info/plain This is the description of the cursor themes. The
first argument is the
original description that comes from the index.theme file.
The second argument is
the list of available sizes, which is never empty. This
string is localized with
support for plural forms: You can make different
singular/plural translations
depending on the number (!) of list items in the second
argument.,
%1 (Size: %2)/* only 1 item in sizeListString
*/
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeliststring).arg(sizeList.size());
I'll test this after holiday.
Chusslove Illich wrote:
i18ncp() should not be used for clever tricks. The plural string should
always be the direct pluralization of the singular string.
I think it would be just fine to leave it at the original version, with
...
Available sizes: ... even if there is only one size. Also no need to
mention in the context that list of sizes is never empty.
Otherwise, use two separate messages with ordinary if-selection.
What's about merging both approaches?
int sizeListSize = sizeList.size();
QString sizeListString = QString::number(sizeList.takeFirst());
while (!sizeList.isEmpty())
{
sizeListString.append(, );
sizeListString.append(QString::number(sizeList.takeFirst()));
};
if (sizeList.size() == 1)
m_description = i18ncp(
@info/plain This is the description of the cursor themes. The
first argument is
the original description that comes from the index.theme
file. The second
argument is the size (in pixel). Example:
'OriginalDescription (Size: 24)',
%1 (Size: %2)).arg(m_description).arg(sizeListString);
else // sizeList.size() 1
/* The translation is aware of plural forms. i18ncp uses the first
integer argument to
distinguish plural forms. The first and the second argument are
QString. So we use
sizeList.size() as third argument to provide proper support for
plural forms. This
works, although it is never referenced with %3 in the string
itself. Although we
provide english strings only for 1 item and for more than 1
item, but i18ncp
will silently expand to as many plural forms as necesarry in the
target language. */
m_description = i18ncp(
@info/plain This is the description of the cursor themes. The
first argument is
the original description that comes from the index.theme
file. The second
argument is the list of available sizes (in pixel). This
string is localized
with support for plural forms: You can make different
singular/plural
translations depending on the number (!) of list items in
the second
argument. Example: 'OriginalDescription (Available sizes:
24, 36, 48)' has
the plural form for 3 because the list has 3 items.,
%1 (Available size: %2) /* only 1 item in sizeListString -
will never be used */
%1 (Available sizes: %2) /* more than 1 item in
sizeListString */
).arg(m_description).arg(sizeListString).arg(sizeListSize);
This way we have a good translation for only 1 size: (Size: 24). In this case
the user can't do anything but use this size. And in the case of more than 1
size, the user can determine himself which size he wants to use
Review Request: Make mouse cursor size configurable
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101701/ --- Review request for KDE Base Apps, KDE Runtime, kdelibs, and Christoph Feck. Summary --- X11 mouse cursor themes can contain cursors in multiple sizes, making them pseudo-scalable. It is yet possible in KDE to configure manually the mouse cursor size (editing kcminput.rc). However, the GUI of the corresponding KControl module didn't provide support to change this. This patch add support for changing the mouse cursor size to the GUI. This are mostly GUI related changes. The underlying data structure XCursorTheme did yet provide support for choosing different sizes and only needed some adjustments. This addresses bug 90444. http://bugs.kde.org/show_bug.cgi?id=90444 Diffs - kcontrol/input/xcursor/xcursortheme.cpp a987487 kcontrol/input/xcursor/themepage.ui 2e38054 kcontrol/input/xcursor/xcursortheme.h b474086 kcontrol/input/xcursor/themepage.h 902148f kcontrol/input/xcursor/themepage.cpp 24b9efe kcontrol/input/xcursor/previewwidget.h f4d2c4e kcontrol/input/xcursor/previewwidget.cpp 3c264fc kcontrol/input/xcursor/cursortheme.h 8f7834b kcontrol/input/xcursor/legacytheme.h 23c9d5f Diff: http://git.reviewboard.kde.org/r/101701/diff Testing --- Tested locally. Works fine for me. Also when using non-standard font DPI values. Screenshots --- http://git.reviewboard.kde.org/r/101701/s/188/ http://git.reviewboard.kde.org/r/101701/s/189/ Thanks, Lukas
Help with revert commit
Hello. The commit 9e1de272620da40fead11dfdb705cf c3d9a90f10 that I've done this morning should not go to master because this would mean a new feature in 4.7 and it changes a localized string. For some reasons, since hours I don't get a working git connection (fatal: read error: Connection reset by peer), and I don't get it reverted. But this commit should be reverted soon - before the i18n scripts run. Could someone help me and revert the commit? Sorry for inconvenience. (Will not happen another time; I promise.) Regards. Lukas Sommer
Re: Help with revert commit
Finally I've managed to get my git working and revert the commit. Sorry for inconvenience. Lukas Sommer 2011/6/11 Lukas Sommer sommer...@gmail.com Hello. The commit 9e1de272620da40fead11dfdb705cf c3d9a90f10 that I've done this morning should not go to master because this would mean a new feature in 4.7 and it changes a localized string. For some reasons, since hours I don't get a working git connection (fatal: read error: Connection reset by peer), and I don't get it reverted. But this commit should be reverted soon - before the i18n scripts run. Could someone help me and revert the commit? Sorry for inconvenience. (Will not happen another time; I promise.) Regards. Lukas Sommer
Re: Review Request: startkde.cmake should treat font dpi like kcontrol/krdb/krdb.cpp does
On June 6, 2011, 2:10 a.m., David Faure wrote: Looks good. Do you want me to commit this for you, or do you want to get a kde contributor account instead, to be able to commit this directly as well as future patches? :) It would be great to have a kde contributor account. (However, I will have to learn to work with GIT. :) - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101410/#review3703 --- On June 1, 2011, 10:54 a.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101410/ --- (Updated June 1, 2011, 10:54 a.m.) Review request for KDE Base Apps, KDE Runtime and kdelibs. Summary --- I've been experimenting a little bit with custom font dpi sizes in $HOME/.kde4/share/config/kcmfonts There, you can set custom font dpi value. Although the user interface (systemsettings/fonts) only provides 0 (don't force a dpi, use the system default instead), 96 and 120 as choice, in the config file you can put (manually) arbitrary values. This works mostly fine because kcontrol/krdb/krdb.cpp simply processes the dpi value of the config file as is. However, startkde.cmake doesn't. It checks if the value is 96 or 120. If not, the value is ignored and the system default is used. Result: When you use e.g. 200 as dpi value and restart KDE, then KWin uses the default value (96 dpi on my system) for the window title, while the applications themself are displayed using 200 dpi. So the applications fonts are as big as desired, but the window title is too small. See the attached screenshot. This behaviour is inconsistent. startkde.cmake should follow the same police as kcontrol/krdb/krdb.cpp does. This patch fixes this. (The user interface could be adopted in another patch.) (Make this work would benefit people who need a high display resolution. Example: You connect your computer to your plasma tv and you want to be still be able to read the text although you are at a distance of 5 meters from the tv monitor. See also bug 272266) This addresses bug 190489. http://bugs.kde.org/show_bug.cgi?id=190489 Diffs - kcontrol/fonts/fonts.h 2c722d4 kcontrol/fonts/fonts.cpp 0cd2666 startkde.cmake dde9c23 Diff: http://git.reviewboard.kde.org/r/101410/diff Testing --- I've applied the patch to my local /usr/bin/startkde file, and it works fine. Screenshots --- http://git.reviewboard.kde.org/r/101410/s/171/ Thanks, Lukas
Re: Review Request: startkde.cmake should treat font dpi like kcontrol/krdb/krdb.cpp does
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101410/ --- (Updated June 1, 2011, 10:04 a.m.) Review request for KDE Base Apps, KDE Runtime and kdelibs. Changes --- Updated diff. The new diff includes the corresponding changes for the GUI, with the following characteristicas: - Default is not forcing any DPI value - If the user wants, he can choose dpi values from 1 dpi up to 1000 dpi. (This is a little bit arbitrary. However, QSpinBox's default maximum value is 99, and this would not be sufficient.) - Each step in the spinbox are 24 dpi, because the common covenience values 72, 96 and 120 are multiples of 24. Going in steps of 24 maybe makes fonts look better. However, the user can choose manually any value ... - I've notices that the DPI setting is also available on MS Windows, but hasn't any effect there because Xft isn't available. Maybe this field should not build on Windows? (Some #ifdef ...?) - There is no live preview of the dpi value for the fonts. Maybe this should be done in a seperate patch? Summary --- I've been experimenting a little bit with custom font dpi sizes in $HOME/.kde4/share/config/kcmfonts There, you can set custom font dpi value. Although the user interface (systemsettings/fonts) only provides 0 (don't force a dpi, use the system default instead), 96 and 120 as choice, in the config file you can put (manually) arbitrary values. This works mostly fine because kcontrol/krdb/krdb.cpp simply processes the dpi value of the config file as is. However, startkde.cmake doesn't. It checks if the value is 96 or 120. If not, the value is ignored and the system default is used. Result: When you use e.g. 200 as dpi value and restart KDE, then KWin uses the default value (96 dpi on my system) for the window title, while the applications themself are displayed using 200 dpi. So the applications fonts are as big as desired, but the window title is too small. See the attached screenshot. This behaviour is inconsistent. startkde.cmake should follow the same police as kcontrol/krdb/krdb.cpp does. This patch fixes this. (The user interface could be adopted in another patch.) (Make this work would benefit people who need a high display resolution. Example: You connect your computer to your plasma tv and you want to be still be able to read the text although you are at a distance of 5 meters from the tv monitor. See also bug 272266) Diffs (updated) - kcontrol/fonts/fonts.h 2c722d4 kcontrol/fonts/fonts.cpp 0cd2666 startkde.cmake dde9c23 Diff: http://git.reviewboard.kde.org/r/101410/diff Testing --- I've applied the patch to my local /usr/bin/startkde file, and it works fine. Screenshots --- http://git.reviewboard.kde.org/r/101410/s/171/ Thanks, Lukas
Re: Review Request: startkde.cmake should treat font dpi like kcontrol/krdb/krdb.cpp does
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101410/ --- (Updated June 1, 2011, 10:54 a.m.) Review request for KDE Base Apps, KDE Runtime and kdelibs. Summary --- I've been experimenting a little bit with custom font dpi sizes in $HOME/.kde4/share/config/kcmfonts There, you can set custom font dpi value. Although the user interface (systemsettings/fonts) only provides 0 (don't force a dpi, use the system default instead), 96 and 120 as choice, in the config file you can put (manually) arbitrary values. This works mostly fine because kcontrol/krdb/krdb.cpp simply processes the dpi value of the config file as is. However, startkde.cmake doesn't. It checks if the value is 96 or 120. If not, the value is ignored and the system default is used. Result: When you use e.g. 200 as dpi value and restart KDE, then KWin uses the default value (96 dpi on my system) for the window title, while the applications themself are displayed using 200 dpi. So the applications fonts are as big as desired, but the window title is too small. See the attached screenshot. This behaviour is inconsistent. startkde.cmake should follow the same police as kcontrol/krdb/krdb.cpp does. This patch fixes this. (The user interface could be adopted in another patch.) (Make this work would benefit people who need a high display resolution. Example: You connect your computer to your plasma tv and you want to be still be able to read the text although you are at a distance of 5 meters from the tv monitor. See also bug 272266) This addresses bug 190489. http://bugs.kde.org/show_bug.cgi?id=190489 Diffs - kcontrol/fonts/fonts.h 2c722d4 kcontrol/fonts/fonts.cpp 0cd2666 startkde.cmake dde9c23 Diff: http://git.reviewboard.kde.org/r/101410/diff Testing --- I've applied the patch to my local /usr/bin/startkde file, and it works fine. Screenshots --- http://git.reviewboard.kde.org/r/101410/s/171/ Thanks, Lukas
Re: Review Request: startkde.cmake should treat font dpi like kcontrol/krdb/krdb.cpp does
On May 25, 2011, 10:19 a.m., Christoph Feck wrote: The big problem with this patch is that the user interface only offers a ComboBox with two fixed values. What does the interface show when you modified the value to, say, 200 dpi? Well, the user interface offers three different values: 96 dpi, 120 dpi and disabled. Any value different from 96 and 120 will be interpreted by the user interface as disabled. However, kcontrol/krdb/krdb.cpp does yet process all values, also when they are not 96 or 120. Should I also come up with a separate patch to the UI? Or integrate a patch to the UI in this patch? - Lukas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101410/#review3505 --- On May 22, 2011, 9:48 a.m., Lukas Sommer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101410/ --- (Updated May 22, 2011, 9:48 a.m.) Review request for KDE Base Apps, KDE Runtime and kdelibs. Summary --- I've been experimenting a little bit with custom font dpi sizes in $HOME/.kde4/share/config/kcmfonts There, you can set custom font dpi value. Although the user interface (systemsettings/fonts) only provides 0 (don't force a dpi, use the system default instead), 96 and 120 as choice, in the config file you can put (manually) arbitrary values. This works mostly fine because kcontrol/krdb/krdb.cpp simply processes the dpi value of the config file as is. However, startkde.cmake doesn't. It checks if the value is 96 or 120. If not, the value is ignored and the system default is used. Result: When you use e.g. 200 as dpi value and restart KDE, then KWin uses the default value (96 dpi on my system) for the window title, while the applications themself are displayed using 200 dpi. So the applications fonts are as big as desired, but the window title is too small. See the attached screenshot. This behaviour is inconsistent. startkde.cmake should follow the same police as kcontrol/krdb/krdb.cpp does. This patch fixes this. (The user interface could be adopted in another patch.) (Make this work would benefit people who need a high display resolution. Example: You connect your computer to your plasma tv and you want to be still be able to read the text although you are at a distance of 5 meters from the tv monitor. See also bug 272266) Diffs - startkde.cmake dde9c23 Diff: http://git.reviewboard.kde.org/r/101410/diff Testing --- I've applied the patch to my local /usr/bin/startkde file, and it works fine. Screenshots --- http://git.reviewboard.kde.org/r/101410/s/171/ Thanks, Lukas
Review Request: startkde.cmake should treat font dpi like kcontrol/krdb/krdb.cpp does
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101410/ --- Review request for KDE Base Apps, KDE Runtime and kdelibs. Summary --- I've been experimenting a little bit with custom font dpi sizes in $HOME/.kde4/share/config/kcmfonts There, you can set custom font dpi value. Although the user interface (systemsettings/fonts) only provides 0 (don't force a dpi, use the system default instead), 96 and 120 as choice, in the config file you can put (manually) arbitrary values. This works mostly fine because kcontrol/krdb/krdb.cpp simply processes the dpi value of the config file as is. However, startkde.cmake doesn't. It checks if the value is 96 or 120. If not, the value is ignored and the system default is used. Result: When you use e.g. 200 as dpi value and restart KDE, then KWin uses the default value (96 dpi on my system) for the window title, while the applications themself are displayed using 200 dpi. So the applications fonts are as big as desired, but the window title is too small. See the attached screenshot. This behaviour is inconsistent. startkde.cmake should follow the same police as kcontrol/krdb/krdb.cpp does. This patch fixes this. (The user interface could be adopted in another patch.) (Make this work would benefit people who need a high display resolution. Example: You connect your computer to your plasma tv and you want to be still be able to read the text although you are at a distance of 5 meters from the tv monitor. See also bug 272266) Diffs - startkde.cmake dde9c23 Diff: http://git.reviewboard.kde.org/r/101410/diff Testing --- I've applied the patch to my local /usr/bin/startkde file, and it works fine. Screenshots --- http://git.reviewboard.kde.org/r/101410/s/171/ Thanks, Lukas
Re: Review Request: Better documentation for KConfigDialog::addPage()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101409/ --- (Updated May 21, 2011, 11:03 a.m.) Review request for kdelibs. Changes --- Updated diff, using the text from Aaron. Summary --- The documentation of KConfigDialog::addPage() is not very detailed about the parameter pixmapName. This patch adds to the documentation that it is possible to simply pass the KDE icon name instead of the pixmap path. This addresses bug 273723. http://bugs.kde.org/show_bug.cgi?id=273723 Diffs (updated) - kdeui/dialogs/kconfigdialog.h 873460b Diff: http://git.reviewboard.kde.org/r/101409/diff Testing --- Thanks, Lukas
Review Request: Better documentation for KConfigDialog::addPage()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101409/ --- Review request for kdelibs. Summary --- The documentation of KConfigDialog::addPage() is not very detailed about the parameter pixmapName. This patch adds to the documentation that it is possible to simply pass the KDE icon name instead of the pixmap path. This addresses bug 273723. http://bugs.kde.org/show_bug.cgi?id=273723 Diffs - kdeui/dialogs/kconfigdialog.h 873460b Diff: http://git.reviewboard.kde.org/r/101409/diff Testing --- Thanks, Lukas
