fixed an issue
ClosedPublic

Authored by grebois on May 1 2016, 11:28 AM.

Details

Summary

before, when we cleared history by context menu, the listkeyword was deleted too
now only the history is deleted.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
grebois updated this revision to Diff 3589.May 1 2016, 11:28 AM
grebois retitled this revision from to fixed an issue.
grebois updated this object.
grebois edited the test plan for this revision. (Show Details)
grebois added reviewers: mlaurent, bensi, ervin.
grebois added a project: KDE PIM: KMail.
grebois added a subscriber: hrouis.
ervin added inline comments.May 1 2016, 1:44 PM
pimcommon/src/widgets/lineeditwithcompleter.h
38–40

I don't think you need this signal.

Just reimplement slotClearHistory in the LineEditWithKeyword subclass (might require marking it virtual in LineEditWithCompleter though).

pimcommon/src/widgets/lineeditwithkeyword.cpp
18 ↗(On Diff #3589)

Put the opening curly brace '{' on its own line.

40 ↗(On Diff #3589)

Put the opening curly brace '{' on its own line.

46 ↗(On Diff #3589)

Put the opening curly brace '{' on its own line.

pimcommon/src/widgets/lineeditwithkeyword.h
8 ↗(On Diff #3589)

Should probably be "LineEditWithKeywords" since there's more than one. :-)

14–15 ↗(On Diff #3589)

Both can be private in my opinion.

Also, name wise you likely want to write "keyword list" everywhere, not "list keyword" (otherwise doesn't read like english).

16–17 ↗(On Diff #3589)

Try to keep the private at the end of the class definition.

grebois updated this revision to Diff 3591.May 1 2016, 3:06 PM

change LineEditWithKeyword -> LineEditWithKeywords
change listKeyword -> keywordList
addKeywordList and getKeywordList are private
private is at the end
signal removed
virtual added and slotClearHistory reimplemented

it's work, but I have a doubt on the Way Which I used virtual, to confirm.

ervin edited edge metadata.May 1 2016, 3:49 PM

You did it almost correctly for the virtual method, you just missed the chaining with the implementation in the base class to avoid duplicating code.

A couple more nitpicks to satisfy my obsessive compulsive disorder. ;-)

Otherwise getting there in my opinion. Good job.

I'll leave it to Laurent to decide if he likes the overall approach.

pimcommon/src/widgets/lineeditwithcompleter.h
38

No need to introduce a new empty line here.

pimcommon/src/widgets/lineeditwithkeywords.cpp
50–51

Better: replace those two lines with "LineEditWithCompleter::slotClearHistory();"

It'll call the implementation of the base class this way, you won't need to duplicate it.

pimcommon/src/widgets/lineeditwithkeywords.h
15

Keep it private too. Add Q_DECL_OVERRIDE at the end of the line (just before the ';'.

grebois updated this revision to Diff 3592.May 1 2016, 4:24 PM
grebois edited edge metadata.

changing both lines by LineEditWithCompleter::slotClearHistory();
i have to change private Q_SLOTS in public Q_SLOTS in LineEditWithCompleter.h
it's still ok ?

ervin added inline comments.May 1 2016, 4:35 PM
pimcommon/src/widgets/lineeditwithcompleter.h
36–37

protected would be enough, no need to turn it fully public.

grebois updated this revision to Diff 3593.May 1 2016, 4:39 PM

protected Q_SLOTS

ervin added a comment.May 1 2016, 6:00 PM

Cool, looks good enough for me. I'll let Laurent weight it on the overall KMail context.

mlaurent added inline comments.May 2 2016, 2:07 PM
pimcommon/src/widgets/lineeditwithkeywords.cpp
2

Missing copyrights

9

why don't assign directly QStringlist ?

> initKeyWordList()

{

keywordlist << i18n(....)... ?

}

you create a private class to assign to keywordlist in other method.

18

Use const here

pimcommon/src/widgets/lineeditwithkeywords.h
2

missing copyright

19

mKeywordList (member variable) => m...

19

It's better to create private class if we want to keep BC as you install this file.

grebois updated this revision to Diff 3606.May 2 2016, 3:00 PM

copyrights added
assign directly keywordList

but i don't understand "It's better to create private class if we want to keep BC as you install this file."

ervin added a comment.May 2 2016, 3:25 PM

Laurent means Binary Compatibility (BC), welcome to the wonderful world of making not only applications but libraries (pimcommon being such a beast). ;-)

More details on binary compatibility in C++:
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

grebois updated this revision to Diff 3621.May 3 2016, 1:10 PM

using d-pointer

mlaurent added inline comments.May 3 2016, 1:57 PM
pimcommon/src/widgets/lineeditwithkeywords.h
34

now that you use private class you can hide addKeyworkList and initKeywordlist into private class

> you clean up method that we can't use in this class

> we clean up header.

grebois updated this revision to Diff 3623.May 3 2016, 3:25 PM

clean up header

mlaurent edited edge metadata.May 3 2016, 3:33 PM

Fix two comment and ship it.
Perhaps in next step for future patch you can create an unittest against this class for testing that we have always list + when we add new item list is not overwrite etc.

Thanks

pimcommon/src/widgets/lineeditwithkeywords.cpp
28

Not really necessary as it's initialize by constructor to empty

pimcommon/src/widgets/lineeditwithkeywords.h
18

use:
"
#ifndef LINEEDITWITHKEYWORDS_H
#define LINEEDITWITHKEYWORDS_H

it's better as name

mlaurent accepted this revision.May 3 2016, 3:33 PM
mlaurent edited edge metadata.
This revision is now accepted and ready to land.May 3 2016, 3:33 PM
aacid closed this revision.Feb 26 2017, 10:26 PM
aacid added a subscriber: aacid.

AFAICS this or a variant of this was commited, closing.