[Kde-accessibility] Review Request 121210: Provide an accessible name for KLed

Discussion:

José Millán Soto

2014-11-22 16:32:03 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/
-----------------------------------------------------------

Review request for KDE Accessibility and KDE Frameworks.

Repository: kwidgetsaddons

Description
-------

This patch will provide a default accessible name for KLed. That name will only say that the widget is a led and the status of the led. If another accessible name has been assigned, it will not be overwritten.
The accessible name will allow accessible tools (such as screen readers) to provide some infomation about the widget.

Diffs
-----

src/kled.h eeb1209
src/kled.cpp 9788fc2

Diff: https://git.reviewboard.kde.org/r/121210/diff/

Testing
-------

Thanks,

JosÃ© MillÃ¡n Soto

David Gil Oliva

2014-11-22 16:40:08 UTC

Permalink

Post by JosÃ© MillÃ¡n Soto
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121210/
-----------------------------------------------------------
(Updated Nov. 22, 2014, 4:32 p.m.)
Review request for KDE Accessibility and KDE Frameworks.
Repository: kwidgetsaddons
Description
-------
This patch will provide a default accessible name for KLed. That name will only say that the widget is a led and the status of the led. If another accessible name has been assigned, it will not be overwritten.
The accessible name will allow accessible tools (such as screen readers) to provide some infomation about the widget.
Diffs
-----
src/kled.h eeb1209
src/kled.cpp 9788fc2
Diff: https://git.reviewboard.kde.org/r/121210/diff/
Testing
-------
Thanks,
JosÃ© MillÃ¡n Soto

José Millán Soto

2014-11-22 16:48:05 UTC

Permalink

Post by JosÃ© MillÃ¡n Soto

src/kled.h, line 257
<https://git.reviewboard.kde.org/r/121210/diff/1/?file=329497#file329497line257>
It should be hidden behind the d-pointer.

Why should it be hidden?
As it is not a virtual function it should not cause binary incompatibility.

- JosÃ©

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/#review70783
-----------------------------------------------------------

David Gil Oliva

2014-11-22 17:31:32 UTC

Permalink

Post by JosÃ© MillÃ¡n Soto

src/kled.h, line 257
<https://git.reviewboard.kde.org/r/121210/diff/1/?file=329497#file329497line257>
It should be hidden behind the d-pointer.

Why should it be hidden?
As it is not a virtual function it should not cause binary incompatibility.

I tend to think That everything private should be behind the d-pointer. It keeps a tighter encapsulation. But maybe I'm wrong...

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/#review70783
-----------------------------------------------------------

Christoph Feck

2014-11-22 18:39:15 UTC

Permalink

Post by JosÃ© MillÃ¡n Soto

src/kled.h, line 257
<https://git.reviewboard.kde.org/r/121210/diff/1/?file=329497#file329497line257>
It should be hidden behind the d-pointer.

Why should it be hidden?
As it is not a virtual function it should not cause binary incompatibility.
I tend to think That everything private should be behind the d-pointer. It keeps a tighter encapsulation. But maybe I'm wrong...

Generally, I agree, but the Private class currently has no q-pointer back to the widget, and I do not think it is worth adding it just for this single function.

- Christoph

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/#review70783
-----------------------------------------------------------

Christoph Feck

2014-11-22 18:31:26 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/#review70793
-----------------------------------------------------------

I think the updateAccessibleName() should also be called from the constructors? Otherwise, the initial name (for which even a constructor exists), is never announced.

src/kled.cpp
<https://git.reviewboard.kde.org/r/121210/#comment49502>

The capitalization "Led" is used in Qt coding style, but the correct term is "LED". Please use all caps in user-visible strings.

src/kled.cpp
<https://git.reviewboard.kde.org/r/121210/#comment49504>

Please use kdelibs coding style:

if () {
...
} else {
...
}

Or, in this case, use a ?: operator, as is used elsewhere in this file.

src/kled.cpp
<https://git.reviewboard.kde.org/r/121210/#comment49503>

Please do not call this here, this function is called for any changes affecting the look.

There are only two functions which change the state, toggle() and setState(). Move it there.

- Christoph Feck

José Millán Soto

2014-11-23 16:09:16 UTC

Permalink

Post by JosÃ© MillÃ¡n Soto

Post by Christoph Feck
I think the updateAccessibleName() should also be called from the constructors? Otherwise, the initial name (for which even a constructor exists), is never announced.

In the new version of the patch, it's called in the constructors. In the old one it was not called there as the constructors called setColor, which called updateCachedPixmap, which called updateAccessibleName.

- JosÃ©

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/#review70793
-----------------------------------------------------------

Post by JosÃ© MillÃ¡n Soto
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121210/
-----------------------------------------------------------
(Updated Nov. 23, 2014, 4:09 p.m.)
Review request for KDE Accessibility, KDE Frameworks and Christoph Feck.
Repository: kwidgetsaddons
Description
-------
This patch will provide a default accessible name for KLed. That name will only say that the widget is a led and the status of the led. If another accessible name has been assigned, it will not be overwritten.
The accessible name will allow accessible tools (such as screen readers) to provide some infomation about the widget.
Diffs
-----
src/kled.cpp 9788fc2
src/kled.h eeb1209
Diff: https://git.reviewboard.kde.org/r/121210/diff/
Testing
-------
Thanks,
JosÃ© MillÃ¡n Soto

José Millán Soto

2014-11-22 18:39:44 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/
-----------------------------------------------------------

(Updated Nov. 22, 2014, 6:39 p.m.)

Review request for KDE Accessibility, KDE Frameworks and Christoph Feck.

Repository: kwidgetsaddons

Description
-------

This patch will provide a default accessible name for KLed. That name will only say that the widget is a led and the status of the led. If another accessible name has been assigned, it will not be overwritten.
The accessible name will allow accessible tools (such as screen readers) to provide some infomation about the widget.

Diffs
-----

src/kled.h eeb1209
src/kled.cpp 9788fc2

Diff: https://git.reviewboard.kde.org/r/121210/diff/

Testing
-------

Thanks,

JosÃ© MillÃ¡n Soto

José Millán Soto

2014-11-23 16:09:10 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/
-----------------------------------------------------------

(Updated Nov. 23, 2014, 4:09 p.m.)

Review request for KDE Accessibility, KDE Frameworks and Christoph Feck.

Changes
-------

New version of the patch, this time solving the issues pointed by Christoph Feck.

Repository: kwidgetsaddons

Description
-------

This patch will provide a default accessible name for KLed. That name will only say that the widget is a led and the status of the led. If another accessible name has been assigned, it will not be overwritten.
The accessible name will allow accessible tools (such as screen readers) to provide some infomation about the widget.

Diffs (updated)
-----

src/kled.cpp 9788fc2
src/kled.h eeb1209

Diff: https://git.reviewboard.kde.org/r/121210/diff/

Testing
-------

Thanks,

JosÃ© MillÃ¡n Soto

Christoph Feck

2014-11-24 01:07:57 UTC

Permalink

Post by JosÃ© MillÃ¡n Soto
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121210/
-----------------------------------------------------------
(Updated Nov. 23, 2014, 4:09 p.m.)
Review request for KDE Accessibility, KDE Frameworks and Christoph Feck.
Repository: kwidgetsaddons
Description
-------
This patch will provide a default accessible name for KLed. That name will only say that the widget is a led and the status of the led. If another accessible name has been assigned, it will not be overwritten.
The accessible name will allow accessible tools (such as screen readers) to provide some infomation about the widget.
Diffs
-----
src/kled.cpp 9788fc2
src/kled.h eeb1209
Diff: https://git.reviewboard.kde.org/r/121210/diff/
Testing
-------
Thanks,
JosÃ© MillÃ¡n Soto

José Millán Soto

2015-01-11 19:11:06 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121210/
-----------------------------------------------------------

(Updated Jan. 11, 2015, 7:11 p.m.)

Status
------

This change has been marked as submitted.

Review request for KDE Accessibility, KDE Frameworks and Christoph Feck.

Repository: kwidgetsaddons

Description
-------

This patch will provide a default accessible name for KLed. That name will only say that the widget is a led and the status of the led. If another accessible name has been assigned, it will not be overwritten.
The accessible name will allow accessible tools (such as screen readers) to provide some infomation about the widget.

Diffs
-----

src/kled.cpp 9788fc2
src/kled.h eeb1209

Diff: https://git.reviewboard.kde.org/r/121210/diff/

Testing
-------

Thanks,

JosÃ© MillÃ¡n Soto