Skip to content

commit for pull request#1

Open
Dmi4er4 wants to merge 1 commit into
mainfrom
true_main
Open

commit for pull request#1
Dmi4er4 wants to merge 1 commit into
mainfrom
true_main

Conversation

@Dmi4er4
Copy link
Copy Markdown
Collaborator

@Dmi4er4 Dmi4er4 commented May 7, 2022

Тот факт, что мне пришлось делать pull request уже можно считать своего рода ревью) Не знаю как лучше стоило его сделать, так что придется оставлять общие комментарии без отсылки к коду

@Dmi4er4
Copy link
Copy Markdown
Collaborator Author

Dmi4er4 commented May 7, 2022

MainWindow.h:

  1. По кодстайлу include должны быть отсортированы по алфавиту;
  2. Лишний пробел перед Q_OBJECT также нежелателен по кодстайлу;
  3. Метод CreateTaskWidgets не имеет реализации, стоило его убрать;

@Dmi4er4
Copy link
Copy Markdown
Collaborator Author

Dmi4er4 commented May 7, 2022

MainWindow.cpp:

  1. Resize на конкретное количество пикселей - не лучшее решение со стороны кросс-платформенности. Здесь лучше было бы воспользоваться встроенными в QT методами для определения размера экрана устройства.
  2. SetWindowIcon не сработал на моем девайсе, так как путь к картинке выставлен не относительно, а глобально. Лучше было бы воспользоваться QResources для хранения иконки прямо в исполняемом файле.
  3. Выстанавливать размер кнопки вручную - достаточно кустарное решение. Лучше было бы взять его относительно размера окна.
  4. Кнопка "Да" в диалоговом окне приложения не закрывает его, вероятно, из-за того, что exec() применен не к главному окну, а к диалогу.
  5. Оставлены комментарии, не несущие смысловой нагрузки для читателя.
  6. Шорткат не работает ни в какой раскладке клавиатуры, кроме английской. Можно было поправить, используя встроенные в QT методы для получения клавиш.

@elizabethfeden
Copy link
Copy Markdown

Материал для ревью, конечно, не особо интересный, но зато в плане объема тебе повезло :)

В целом, нормально, но несколько важных моментов не отмечено:

  • Диалог стоило создавать при помощи QMessageBox::question, это гораздо проще и меньше мест, где можно ошибиться
  • Кнопка "да" не работает из-за неправильного коннекта. Вот как это можно было исправить на скорую руку, хотя и не совсем красиво:
connect(yes_button, &QPushButton::clicked, this, [] {
    exit(0);
  });

Но на самом деле, если попробовать пофиксить кнопку нормально, то вскрывается еще несколько проблем:

  • Устаревший синтаксис коннекта мешает правильно понимать, что вызывается и к чему применяется. В существующем коде у диалога (а не у окна) вызывается функция quit (и qt даже кидает runtime-ошибку, что такой функции нет; если пользоваться новым синтаксисом, ошибка вскроется уже во время компиляции)
  • Если попробовать вызвать close у окна, то это тоже не сработает. Можно подумать, что причина в том, что мы переопределили closeEvent (при этом, кстати, не написав override и не приняв соответствующий event). Но на самом деле все еще интереснее: close можно делать только виджету, который на данный момент виден, а у нас это диалог. Здесь мы приходим к тому, что коннектить кнопки диалога не совсем правильно, и лучше возращать результат нажатия, как это делает QMessageBox::question. Ну и переименовать closeEvent так, чтобы не было неожиданных пересечений с родительским классом.

Ну и еще вещи, которые не так важны, но тоже можно было упомянуть:

  • Названия кнопок/лейблов можно и нужно писать в конструкторе (т. е. делать new QPushButton(parent, name));
  • resize + move можно было склеить в один метод setGeometry;
  • шорткат правильнее не коннектить, а пользоваться методом button->setShortcut

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