Skip to content

Add group selection and deletion operations - #31

Open
rakhmukova wants to merge 4 commits into
mainfrom
groupOperations
Open

Add group selection and deletion operations#31
rakhmukova wants to merge 4 commits into
mainfrom
groupOperations

Conversation

@rakhmukova

Copy link
Copy Markdown
Collaborator

No description provided.

@Alexander-Ploskin

Copy link
Copy Markdown
Collaborator

В целом круто, есть несколько моментов, на которые бы хотелось обратить внимание:

  • хочется выделять не только по пкм, но и по дабл клику
  • ребра сами по себе не выделяются, хотя не очень понятно почему, нужно добавить возможность выделять ребра тоже
  • было бы неплохо переделать стиль хайлайта элементов, а то сейчас он не сильно отличается от выделения при рисовке ребер
  • я думаю, то стоит выделять вершины и в других режимах тоже, например, в удалении точно стоит, это же удобно

Comment on lines +22 to +26
public Rect SelectedRect
{
get => selectedRect;
set => selectedRect = value;
}

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.

Так можно же SelectRect { get; set; }, переопределять геттер и сеттер стоит только если хочется какую-то дополнительную логику добавить

@@ -1,4 +1,5 @@
using ControlsLibrary.Controls.ErrorReporter;


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.

Пустые строчки не нужны

Comment on lines +759 to +762
x = mouseDownPosition.X < mouseCurrentPosition.X ?
mouseDownPosition.X : mouseCurrentPosition.X;
y = mouseDownPosition.Y < mouseCurrentPosition.Y ?
mouseDownPosition.Y : mouseCurrentPosition.Y;

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.

Большие тернарники на несколько строк лучше оформлять так:

Suggested change
x = mouseDownPosition.X < mouseCurrentPosition.X ?
mouseDownPosition.X : mouseCurrentPosition.X;
y = mouseDownPosition.Y < mouseCurrentPosition.Y ?
mouseDownPosition.Y : mouseCurrentPosition.Y;
x = mouseDownPosition.X < mouseCurrentPosition.X
? mouseDownPosition.X
: mouseCurrentPosition.X;
y = mouseDownPosition.Y < mouseCurrentPosition.Y
? mouseDownPosition.Y
: mouseCurrentPosition.Y;

Comment on lines -625 to +680
private void SafeRemoveVertex(VertexControl vc)
private void TrySafeRemoveVertex(VertexControl vc)
{
if (ExecutorViewModel.InSimulation)
{
return;
}
SafeRemoveVertex(vc);
VertexRemoved?.Invoke(this, new VertexRemovedEventArgs() { VertexControl = vc });
}

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.

Гм, как я понял, новый метод явно не нужен. Он же отличается только тем, что ты исправила баг с тем, что вершину можно было удалить при исполнении, так это же и в старом методе нужно было исправить. А новое событие VertexRemoved и там бы не помешало рейзить.

Comment on lines +582 to +586
private void zoomControl_PropertyChanged(object sender, PropertyChangedEventArgs e)
{
zoomControl.TranslateX = initialTranslateX;
zoomControl.TranslateY = initialTranslateY;
}

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.

Надо добавить проверку на то, что было изменено интересующее нас свойство Presenter или как оно там называется

Comment on lines +401 to +410
else if (e.RightButton == MouseButtonState.Pressed)
{
if (Toolbar.SelectedTool == SelectedTool.Select)
{
if (selectedArea != null)
{
ClearSelectedArea();
}
ClearSelectMode(true);
SelectionStarted?.Invoke(this, e);

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.

Ага, стоит добавить дабл клик и, наверное, возможность выделения в других режимах. Насчет едита не уверен, но удаление бы точно не помешало. Хотя система с режимами мне не особо нравится, но пока она жива стоит делать ее удобной.

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