Skip to content

PSO fixes#452

Open
uyraq2001 wants to merge 11 commits into
altlinux:masterfrom
uyraq2001:master
Open

PSO fixes#452
uyraq2001 wants to merge 11 commits into
altlinux:masterfrom
uyraq2001:master

Conversation

@uyraq2001

Copy link
Copy Markdown

No description provided.

@uyraq2001 uyraq2001 force-pushed the master branch 6 times, most recently from 6bad810 to 1a30390 Compare June 12, 2026 01:37
@uyraq2001 uyraq2001 marked this pull request as ready for review June 12, 2026 01:38
* ADMC - AD Management Center
*
* Copyright (C) 2020-2025 BaseALT Ltd.
* Copyright (C) 2020-2025 Dmitry Degtyarev

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you need to put here another line with copyright date and your name, if you are one of the authors of the file.

* ADMC - AD Management Center
*
* Copyright (C) 2020-2025 BaseALT Ltd.
* Copyright (C) 2020-2025 Dmitry Degtyarev

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same comment about the copyright header here.

});
}

void PasswordSettingsImpl::fetch(const QModelIndex &index) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd put a Doxygen comment before methods, to document what the method does, what it accepts as parameters, what it asserts (if it does so) and what it returns. Also, if there are any exceptions that can be thrown in the method, I'd document them too.


ui->applied_list_widget->clear();

ui->name_edit->setReadOnly(true);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It breaks PSO objects UI name edition. Remove that line please. If you wanna setup specific configuration for this widget you can use enum (for ctor) or setter f-n.

@Samael-fhts Samael-fhts Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For global settings, the name field will always be empty. It shouldn't be displayed like other empty settings fields.
Also, the header "Default password settings" would be better for the global case instead of "Password settings".

ui->edit_button->setDisabled(true);
ui->cancel_button->setDisabled(true);
ui->apply_button->setDisabled(true);
pso_edit_widget->set_read_only(true);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If all buttons are disabled, it's better to just hide them.

QAction *create_pso_action;
};

void password_settings_impl_add_objects(ConsoleWidget *console, const QList<AdObject> &object_list, const QModelIndex &parent);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unused function

else if (object.is_class(CLASS_SUBNET)) {
stacked_widget->setCurrentWidget(subnet_results_widget);
subnet_results_widget->update(object);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did u move that commit to another PR?

@artyom-poptsov

Copy link
Copy Markdown
Contributor

Hi!

I think it would be better if we would provide at least some overview of the work we've done in the PR description, aside from the PR title.

Here we have 11 commits and no information provided. That means a reviewer will have hard time figuring out what the author of those changes wanted to accomplish by this work.

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.

3 participants