Skip to content

Fix inheritance of LocationDelegate #2300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daschuer merged 2 commits into from
Sep 27, 2019
Merged

Fix inheritance of LocationDelegate #2300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7229062
Remove forward declarations of Qt classes and fix formatting
uklotzde Sep 26, 2019
cf9b09e
Fix inheritance of LocationDelegate
uklotzde Sep 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions src/library/locationdelegate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
#include <QtDebug>
#include "library/locationdelegate.h"

#include <QTableView>
#include <QPainter>

#include "library/locationdelegate.h"

LocationDelegate::LocationDelegate(QTableView* pTableView)
: QStyledItemDelegate(pTableView),
m_pTableView(pTableView) {
: TableItemDelegate(pTableView) {
}

void LocationDelegate::paint(QPainter* painter, const QStyleOptionViewItem& option,
const QModelIndex& index) const {
QString elidedLocation =
option.fontMetrics.elidedText(index.data().toString(),
Qt::ElideLeft,
m_pTableView->columnWidth(index.column()));
void LocationDelegate::paintItem(
QPainter* painter,
const QStyleOptionViewItem& option,
const QModelIndex& index) const {
QString elidedLocation = option.fontMetrics.elidedText(
index.data().toString(),
Qt::ElideLeft,
columnWidth(index));
painter->drawText(option.rect, Qt::AlignVCenter, elidedLocation);
}
23 changes: 9 additions & 14 deletions src/library/locationdelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
#ifndef LOCATIONDELEGATE_H
#define LOCATIONDELEGATE_H
#pragma once

#include <QStyledItemDelegate>
#include "library/tableitemdelegate.h"

class QTableView;
class QStyleOptionViewItem;

class LocationDelegate : public QStyledItemDelegate {
class LocationDelegate : public TableItemDelegate {
Q_OBJECT
public:
LocationDelegate(QTableView* pTrackTable);
explicit LocationDelegate(QTableView* pTrackTable);
~LocationDelegate() override = default;

void paint(QPainter* painter, const QStyleOptionViewItem& option,
const QModelIndex& index) const;

private:
QTableView* m_pTableView;
void paintItem(
QPainter* painter,
const QStyleOptionViewItem& option,
const QModelIndex& index) const override;
};

#endif /* LOCATIONDELEGATE_H */
16 changes: 9 additions & 7 deletions src/library/tableitemdelegate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
#include "library/tableitemdelegate.h"

#include <QTableView>
#include <QPainter>

#include "library/tableitemdelegate.h"


TableItemDelegate::TableItemDelegate(QTableView* pTableView)
: QStyledItemDelegate(pTableView),
m_pTableView(pTableView) {
}

TableItemDelegate::~TableItemDelegate() {
}

void TableItemDelegate::paint(QPainter* painter,const QStyleOptionViewItem& option,
const QModelIndex& index) const {
void TableItemDelegate::paint(
QPainter* painter,
const QStyleOptionViewItem& option,
const QModelIndex& index) const {

painter->save();

Expand Down Expand Up @@ -46,3 +44,7 @@ void TableItemDelegate::paint(QPainter* painter,const QStyleOptionViewItem& opti

painter->restore();
}

int TableItemDelegate::columnWidth(const QModelIndex &index) const {
return m_pTableView->columnWidth(index.column());
}
23 changes: 13 additions & 10 deletions src/library/tableitemdelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,28 @@
#ifndef TABLEITEMDELEGATE_H
#define TABLEITEMDELEGATE_H
#pragma once

#include <QStyledItemDelegate>
#include <QTableView>

class QTableView;
class QStyleOptionViewItem;

class TableItemDelegate : public QStyledItemDelegate {
Q_OBJECT
public:
explicit TableItemDelegate(QTableView* pTableView);
virtual ~TableItemDelegate();
~TableItemDelegate() override = default;
Copy link
Copy Markdown
Member

@daschuer daschuer Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant on one hand, but I remember that there was an issue with a clang compiler warning. Is this still the case? Do we have a number of these compiler warnings elsewhere?
If yes, we should think about a mass fix and drop a note here: https://www.mixxx.org/wiki/doku.php/coding_guidelines

Copy link
Copy Markdown
Contributor Author

@uklotzde uklotzde Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not redundant! It verifies that the destructor of the base class is virtual.

Copy link
Copy Markdown
Member

@daschuer daschuer Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes right, but is this important here?
But nevermind, it is a question if code style. We just need to state somewhere if we want that line or not.

Does this issue a compiler warning, btw?

Copy link
Copy Markdown
Contributor Author

@uklotzde uklotzde Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know and don't really care. If this pattern is not desired then simply remove all lines that match the regex "~.*override = default;" in a separate commit.

Copy link
Copy Markdown
Member

@daschuer daschuer Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have read a bit what others think. While I originally was the opinion that it is unnecessary noise, this post convinced me that it is actually sensible isocpp/CppCoreGuidelines#721 (comment)

I have added a sentence to our style guides:

This applies also for default destructors even if it looks noisy.


void paint(QPainter *painter, const QStyleOptionViewItem &option,
const QModelIndex &index) const;
void paint(
QPainter *painter,
const QStyleOptionViewItem &option,
const QModelIndex &index) const override;

virtual void paintItem(QPainter *painter, const QStyleOptionViewItem &option,
virtual void paintItem(
QPainter *painter,
const QStyleOptionViewItem &option,
const QModelIndex &index) const = 0;

protected:
int columnWidth(const QModelIndex &index) const;

private:
QTableView* m_pTableView;
};

#endif // TABLEITEMDELEGATE_H