Skip to content

Client#6

Open
EremeyR wants to merge 18 commits intomasterfrom
client
Open

Client#6
EremeyR wants to merge 18 commits intomasterfrom
client

Conversation

@EremeyR
Copy link
Collaborator

@EremeyR EremeyR commented Nov 18, 2021

No description provided.

Copy link
Collaborator

@a-badin a-badin left a comment

Choose a reason for hiding this comment

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

Фиксите изменения и сливайте в main

Q_OBJECT

public:
explicit Brush(qreal brushSize = 10, const QColor& brushColor = Qt::red, QWidget *parent = nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

несколько дефолтных значений - это не здорово. Если вы ходите изменить только parent, то первые жва придется продублировать. Оставляйте в параметрах конструктора только необходимое для создания объекта. Если пользователю будет нужно поменять размер - он вызовет сеттер

private slots:
void BrushTriggeredSlot();
private:
qreal brushSize;
Copy link
Collaborator

Choose a reason for hiding this comment

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

brushSize_

~Canvas() = default;

public slots:
void openImageSlot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

слово slot писать в названии необьязательно, а вот параметры напрашиваются



private:
QString filePath = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

QString с конструктором по-умолчанию, поэтому он вызовется автоматически. руками инициализровать не нужно

~Changer() = default;

private:
int _value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это не ошибка, но скорее правило хорошего тона, не создавать идентификаторы, которые начинаются с _
Это есть в книгах и есть в полном стандарте С++. Считается, что имена, которые начинаются с _ и которые содержат два __ зарезервированы для реализации стандартной библиотеки/компилятора/компоновщика


QPoint previous_point;

std::vector<QPoint> curve;
Copy link
Collaborator

Choose a reason for hiding this comment

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

зачем хранить кривую в сцене


public slots:
void SetBrushSizeSlot(int value);
void SetBrushRedSlot(int value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

В Qt наверняка есть какая нибудь структура под цвет, чтобы сократить число сигналов и слотов в 3 раза

Brush::Brush(qreal brushSize, const QColor& brushColor, QWidget *parent) :
brushSize(brushSize), brushColor(brushColor), QAction(parent) {
assert(parent != nullptr);
connect(this, &QAction::triggered, this, &Brush::BrushTriggeredSlot);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Можно связывать сигналы с сигналами, если нужно

QGraphicsView(parent) {
}

void Canvas::openImageSlot() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

отпределено это не ответственность Canvas показывать диалог пользователю, скорее этот класс должен принимать QImage, причем он это делать кажется и так умеет. Логика выбора картинки должна быть где-то выше в MainWindows например

this->setFixedWidth(size_label->width() + spin_box->width() + slider->width() + 20);
std::cout << size_label->width() << " " << spin_box->width() << " " << slider->width();

connect(slider, QOverload<int>::of(&QSlider::valueChanged), this,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Почему нужен overload

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants