KPropertiesDialog: switch to label in setFileNameReadOnly(true)
ClosedPublic

Authored by dfaure on Aug 6 2018, 10:16 PM.

Details

Summary

for consistency, when IconApplet calls this method it shouldn't use
a readonly lineedit, but a QLabel.

Test Plan

locally hacked tests/kpropertiesdialogtest.cpp to add a call to
setFileNameReadOnly(true)

Diff Detail

Repository
R241 KIO
Branch
kpropsdlg
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1608
Build 1626: arc lint + arc unit
dfaure created this revision.Aug 6 2018, 10:16 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 6 2018, 10:16 PM
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald Transcript
dfaure requested review of this revision.Aug 6 2018, 10:16 PM
rkflx added a comment.Aug 7 2018, 5:25 PM

Thanks for the help. Works great, just one inline question about an edge case (everything else LGTM).

src/widgets/kpropertiesdialog.cpp
853

Just out of interest (don't change anything): I assume you don't fully transition everything to d->m_grid to keep the diff small, or because grid is more readable?

1211–1214

Wouldn't this mean that calling setFileNameReadOnly(true) multiple times will also create multiple labels on top of each other, breaking idempotence and resulting in some kind of bold font effect?

dfaure added inline comments.Aug 8 2018, 7:51 AM
src/widgets/kpropertiesdialog.cpp
853

Yeah, a bit of both, mostly to keep the diff small. Most of the usage of grid is here in that method, we need it in a member only for one call, so it seems a bit overkill to use the member everywhere.

1211–1214

Fair point, I'll just add a check that d->m_fileNameLabel doesn't exist already.

dfaure updated this revision to Diff 39291.Aug 8 2018, 7:53 AM

Handle multiple calls to setFileNameReadOnly(true), just in case.

rkflx accepted this revision.Aug 8 2018, 7:55 AM
This revision is now accepted and ready to land.Aug 8 2018, 7:55 AM
dfaure closed this revision.Aug 8 2018, 8:06 AM