Skip to content
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.

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

[Shared Tasks] Cross Zone Remove Fix #1740

Merged
merged 4 commits into from Dec 23, 2021

Conversation

mmcgarvey
Copy link
Contributor

Why:
The cross_zone_remove_task quest methods were not removing from
shared_task_members database table and were not clearing shared task
cache. This resulted in a situation where a character could not
request other shared tasks.

What:
Shamelessly copied shared task logic from ClientTaskState::CancelTask
into ClientTaskState::RemoveTaskByTaskID

Why:
	The cross_zone_remove_task quest methods were not removing from
	shared_task_members database table and were not clearing shared task
	cache.  This resulted in a situation where a character could not
	request other shared tasks.

What:
	Shamelessly copied shared task logic from ClientTaskState::CancelTask
	into ClientTaskState::RemoveTaskByTaskID
Copy link
Contributor

@Kinglykrab Kinglykrab left a comment

Choose a reason for hiding this comment

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

Looks good to me, @hgtw should look it over though.

@Kinglykrab Kinglykrab requested a review from hgtw November 12, 2021 17:13
@Akkadius
Copy link
Member

I’ll take a look at this one later

@hgtw
Copy link
Contributor

hgtw commented Nov 12, 2021

This would work but looking at this method it seems to just mostly duplicate what CancelTask and RemoveTask already do. Could you maybe instead rewrite the method to just forward to CancelTask after finding the task id?

Something like:

if (m_active_task.task_id == task_id)
  CancelTask(client, TASKSLOTTASK, TaskType::Task);
else if (m_active_shared_task.task_id == task_id)
  CancelTask(client, TASKSLOTSHAREDTASK, TaskType::Shared);
else if (m_active_task_count > 0)
  loop m_active_quests to find task_id, if found CancelTask(client, i, TaskType::Quest);

@mmcgarvey
Copy link
Contributor Author

Could you maybe instead rewrite the method to just forward to CancelTask after finding the task id?

Great idea. I will work on that. Thank you.

Copy link
Member

@Akkadius Akkadius left a comment

Choose a reason for hiding this comment

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

@hgtw had a suggestion around re-use

Instead of copying code from CancelTask into RemoveTaskByTaskID, it is better
for code maintenance to simply call CancelTask from RemoveTaskByTaskID.
This is cleaner.

Note:  I chose to be explicit with the remove_from_db parameter, despite true
being the default.  I tend to do this to protect from the default value
changing in the future.
zone/task_client_state.cpp Outdated Show resolved Hide resolved
Removed unused variables.
Distinguished log messages for Shared Tasks from regular Tasks.
@mmcgarvey mmcgarvey requested a review from hgtw November 15, 2021 13:54
@hgtw
Copy link
Contributor

hgtw commented Nov 15, 2021

Thanks for the extra work. I know it went a little beyond the scope of the original pr fix but getting this method refactored and cleaned up is appreciated

@mmcgarvey
Copy link
Contributor Author

Thanks for the extra work. I know it went a little beyond the scope of the original pr fix but getting this method refactored and cleaned up is appreciated

Happy to help. Thank you for your feedback. It will help me in future refactors.

@Kinglykrab
Copy link
Contributor

Any reason this shouldn’t be merged?

@mmcgarvey
Copy link
Contributor Author

Any reason this shouldn’t be merged?

I think I was just waiting on @Akkadius to verify that I have addressed his concerns.

@Akkadius
Copy link
Member

@mmcgarvey sorry for the lag time on this one. Appreciate you following up on feedback

@Akkadius Akkadius merged commit c79fbb9 into EQEmu:master Dec 23, 2021
@Akkadius
Copy link
Member

Also appreciate your PR descriptions and the context you provide

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.

None yet

5 participants