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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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
384

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

384–385

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
384

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.

384–385

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
384

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 36157

View Options

src/settings/ProfileSettings.cpp

Loading...