On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/duplicatefinderjob.h (Diff revision 1)
		class DuplicateFinderJob : public KJob
		80	Result data() const;

A more intuitive name for this function would be result() I think ;)

Ok. :)

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/parsebugbacktraces.h (Diff revision 1)
		class ParseBugBacktraces : QObject
		66	template<class Iter>
		67	ParseBugBacktraces::DuplicateRating rating(Iter it, Iter itEnd, Iter it2, Iter itEnd2);
		68
		69	template<class Iter>
		70	Iter findCrashStackFrame(Iter it, Iter itEnd);
		71
		72	template<class Iter>
		73	Iter increaseIterIfNeeded(Iter it, Iter itEnd);
		74
		75	//TODO improve this stuff, it is just a HACK
		76	template<class Iter>
		77	ParseBugBacktraces::DuplicateRating rating(Iter it, Iter itEnd, Iter it2, Iter itEnd2)
		78	{
		79	int matches = 0;
		80	int lines = 0;
		81
		82	it = findCrashStackFrame(it, itEnd);
		83	it2 = findCrashStackFrame(it2, itEnd2);
		84
		85	while (it != itEnd && it2 != itEnd2) {
		86	if (it->type() == BacktraceLine::StackFrame && it2->type() == BacktraceLine::StackFrame) {
		87	++lines;
		88	if (it->frameNumber() == it2->frameNumber() && it->functionName() == it2->functionName()) {
		89	++matches;
		90	}
		91	++it;
		92	++it2;
		93	continue;
		94	}
		95
		96	//if iters do not point to emptylines or a stackframe increase them
		97	if (it->type() != BacktraceLine::StackFrame && it->type() != BacktraceLine::EmptyLine) {
		98	++it;
		99	continue;
		100	}
		101	if (it2->type() != BacktraceLine::StackFrame && it2->type() != BacktraceLine::EmptyLine) {
		102	++it2;
		103	continue;
		104	}
		105
		106	//one bt is shorter than the other
		107	if (it->type() == BacktraceLine::StackFrame && it2->type() == BacktraceLine::EmptyLine) {
		108	++lines;
		109	++it;
		110	continue;
		111	}
		112	if (it2->type() == BacktraceLine::StackFrame && it->type() == BacktraceLine::EmptyLine) {
		113	++lines;
		114	++it2;
		115	continue;
		116	}
		117
		118	if (it->type() == BacktraceLine::EmptyLine && it2->type() == BacktraceLine::EmptyLine) {
		119	//done
		120	break;
		121	}
		122	}
		123
		124	if (!lines) {
		125	return ParseBugBacktraces::NoDuplicate;
		126	}
		127
		128	const int rating = matches * 100 / lines;
		129	if (rating == 100) {
		130	return ParseBugBacktraces::PerfectDuplicate;
		131	} else if (rating >= 90) {
		132	return ParseBugBacktraces::MostLikelyDuplicate;
		133	} else if (rating >= 60) {
		134	return ParseBugBacktraces::MaybeDuplicate;
		135	} else {
		136	return ParseBugBacktraces::NoDuplicate;
		137	}
		138
		139	return ParseBugBacktraces::NoDuplicate;
		140	}
		141
		142	template<class Iter>
		143	Iter findCrashStackFrame(Iter it, Iter itEnd)
		144	{
		145	Iter result = std::find_if(it, itEnd, boost::bind(&BacktraceLine::type, _1) == BacktraceLine::KCrash);
		146	if (result != itEnd) {
		147	result = std::find_if(result, itEnd, boost::bind(&BacktraceLine::type, _1) == BacktraceLine::StackFrame);
		148	}
		149
		150	return result;
		151	}

These functions aren't really templates. They kind of rely on Iter being a QList<BacktraceLine>::const_iterator. I am ok with making them templates just for avoiding typing the whole QList<BactraceLine>::const_iterator, but actually the same effect could be achieved with a typedef. In any case, they do not need to be in the header. It makes compilation slower and doesn't achieve anything. Just move them in the .cpp, even if you leave them as templates.

The reason I made them templates was that I had problems compiling the code without. Getting errors like

CMakeFiles/drkonqi.dir/duplicatefinderjob.o: In function `rating(QList<BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator, Q List<BacktraceLine>::const_iterator)':
/home/mat-not/kde/src/kde-runtime/drkonqi/parsebugbacktraces.h:76: multiple definition of `rating(QList<BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator, QList< BacktraceLine>::const_iterator, QList<BacktraceLine>::const_iterator)'
CMakeFiles/drkonqi.dir/parsebugbacktraces.o:/home/mat-not/kde/src/kde-runtime/drkonqi/parsebugbacktraces.h:76: first defined here
collect2: ld returned 1 exit status
make[2]: *** [drkonqi/drkonqi] Error 1
make[1]: *** [drkonqi/CMakeFiles/drkonqi.dir/all] Error 2

Unfortunately I haven't sorted these out yet and just used templates instead.

In any case if I leave them templates they have to stay in the header, otherwise it would not compile and if it would it would not be standard conformant.

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/parsebugbacktraces.h (Diff revision 1)
		class ParseBugBacktraces : QObject
		142	template<class Iter>
		143	Iter findCrashStackFrame(Iter it, Iter itEnd)
		144	{
		145	Iter result = std::find_if(it, itEnd, boost::bind(&BacktraceLine::type, _1) == BacktraceLine::KCrash);
		146	if (result != itEnd) {
		147	result = std::find_if(result, itEnd, boost::bind(&BacktraceLine::type, _1) == BacktraceLine::StackFrame);
		148	}
		149
		150	return result;
		151	}

This is quite clever and I like it, but it adds a dependency on boost, which is not an existing dependency of kde-runtime and you haven't added the right checks in CMakeLists.txt either. I would just replace them with for loops if I were you.

Ah, I thought this was used somewhere else.
I'll change that.

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/parsebugbacktraces.cpp (Diff revision 1)
		ParseBugBacktraces::ParseBugBacktraces(const BugReport &bug, QObject *parent)
		28	m_parser = BacktraceParser::newParser("gdb", this);

Yeah, realistically 99% of backtraces on bugzilla are in gdb format, but there can also be kdbgwin ones... But I think for the moment we can live with just gdb.

Indeed, I simply had no time to also implement the kdbgwin ones. :/

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/reportassistantpages_bugzilla_duplicates.cpp (Diff revision 1)
		void BugzillaDuplicatesPage::analyzedDuplicates(KJob *j)
		393	BugReport::Status status = m_result.status;

unused variable?

No it is used below. The only reason I added that was to safe some typing. ;)
If you want I'll remove it in fact.

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/reportassistantpages_bugzilla_duplicates.cpp (Diff revision 1)
		void BugzillaDuplicatesPage::analyzedDuplicates(KJob *j)
		401	item->setBackground(0, brush);
		402	item->setBackground(1, brush);

I take it that you want to color the whole line, right? It would be perhaps a better idea to use item->columnCount() instead of hardcoding.

Yes that is what was my intention.
You mean adding a loop over all columns and to do it there? --> I'll do that.

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/reportassistantpages_bugzilla_duplicates.cpp (Diff revision 1)
		void BugzillaDuplicatesPage::analyzedDuplicates(KJob *j)
		408	i18nc("@label", "Your crash has already been reported as <a href=\"%1\">Bug %1</a>, which is a <strong>duplicate</strong> of <a href=\"%2\">Bug %2</a>", QString::number(duplicate), QString::number(parentDuplicate))) +
		409	'\n' + i18nc("@label", "Only <strong><a href=\"%1\">attach</a></strong> if you can add needed information to the bug report.", QString("attach"));
		410	} else if (BugReport::isClosed(status)) {
		411	text = (parentDuplicate == duplicate ? i18nc("@label", "Your crash has already been reported as <a href=\"%1\">Bug %1</a> which has been <strong>closed</strong>.", QString::number(duplicate)) :
		412	i18nc("@label", "Your crash has already been reported as <a href=\"%1\">Bug %1</a>, which is a duplicate of the <strong>closed</strong> <a href=\"%2\">Bug %2</a>.", QString::number(duplicate), QString::number(parentDuplicate)));

1) The KUIT thing should be "@info:label", iirc.
2) I don't really understand the logic of it. If a report is open or it is RESOLVED/NEEDSINFO, then it can't possibly be marked as a duplicate. What am I missing?

ad 1) I used "@label" since that was already used in the code there. There is also a cheatsheet around, not sure if it is correct though: http://people.ubuntu.com/~apachelogger/misc/i18nccheatsheet.png

ad 2) The idea is that there exists already a bug which is either open or Resolved/Needsinfo, in that case the user should be asked to provide information there if they can. The status of the report itself won't be changed, this line should just lead to enhancing the existing report instead of creating a new one, which would in fact be a duplicate.

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

drkonqi/reportinterface.h (Diff revision 1)
public:
		75	void setDuplicateId(uint);
		76	uint duplicateId() const;

Interesting, looks like this is the only place where a bug report number is a uint. (I think uint is the correct type, but everywhere else it's an int...)

I used uint since ReportInterface::attachToBugNumber also used uint.

On October 19th, 2011, 10:35 p.m., George Kiagiadakis wrote:

(Unrelated comment: Since I still consider myself the maintainer of drkonqi, could you please include me explicitly in the "People" field of review requests related to drkonqi in the future? It makes it easier for me to spot them, since it triggers a filter in my inbox. Thanks.)

Ok, sure. :)

- Matthias

On October 19th, 2011, 6:58 p.m., Matthias Fuchs wrote:

This patch allows DrKonqi to download the possible duplicate bug reports and parse their backtraces.
If a report turns out as duplicate reporting a new bug is not possible anymore.
Thus the number of duplicates reported via DrKonqi should hopefully decline.

Note: The comparing backtraces part is not that good. Unfortunately I do not have much time to find a good algorithm. In the worst case we could make sure that only perfect duplicates are declined, that still should improve the current situation imo.