Differential D13516

simplify isProfileDeletable
ClosedPublic
Actions

Authored by tcanabrava on Jun 13 2018, 1:54 PM.

Details

Reviewers
hindenburg
Group Reviewers
Konsole
Commits
R319:68ddd7b97824: simplify isProfileDeletable
Summary

Maintenability ++

Diff Detail

Repository
R319 Konsole
Branch
smallPartsLogicSimplification
Lint
No Linters Available
Unit
No Unit Test Coverage
tcanabrava created this revision.Jun 13 2018, 1:54 PM
Restricted Application added a project: Konsole. · View Herald TranscriptJun 13 2018, 1:54 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Jun 13 2018, 1:54 PM
hindenburg added a comment.Jun 13 2018, 2:33 PM

Thanks, we put {} around all "if" for consistency and following KDE's style

tcanabrava added a comment.Jun 13 2018, 2:59 PM
In D13516#277839, @hindenburg wrote:

Thanks, we put {} around all "if" for consistency and following KDE's style

I work on kde for 10 years and I *still* forget those silly braces.
ahah. updating.

tcanabrava updated this revision to Diff 36102.Jun 13 2018, 3:00 PM
  • Add missing braces
ahmadsamir added a subscriber: ahmadsamir.Jun 13 2018, 6:54 PM
ahmadsamir added inline comments.
src/settings/ProfileSettings.cpp
385 ↗(On Diff #36102)

I think that if the profile pointer is null, it should return false, otherwise you'd be deleting some unknown .profile.

390 ↗(On Diff #36102)

Same thing here, should be false (if the .profile doesn't exist to begin with it can't be deleted).

tcanabrava added inline comments.Jun 13 2018, 8:09 PM
src/settings/ProfileSettings.cpp
385 ↗(On Diff #36102)

Considering that the code works right now, it’s returning true when we don’t have a profile. While I agree with your reasoning I tried to port the code without changing its behavior.

390 ↗(On Diff #36102)

Same answer, this is the same behavior as before. If the profile doesn’t exists or it’s not writable, we returned trues

ahmadsamir added inline comments.Jun 14 2018, 8:09 AM
src/settings/ProfileSettings.cpp
385 ↗(On Diff #36102)

Yeah, well, this is still a good time to change it, if the change makes more sense.

tcanabrava updated this revision to Diff 36140.Jun 14 2018, 10:25 AM
tcanabrava marked 4 inline comments as done.
  • Fix old logic: don't permit to delete inexistent profiles
hindenburg accepted this revision.Jun 14 2018, 12:31 PM
This revision is now accepted and ready to land.Jun 14 2018, 12:31 PM
Closed by commit R319:68ddd7b97824: simplify isProfileDeletable (authored by tcanabrava, committed by hindenburg). · Explain WhyJun 14 2018, 12:57 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/settings/ProfileSettings.cpp (22 lines)

Diff 36140

View Options

src/settings/ProfileSettings.cpp

Loading...