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

Fix not using custom chatbox's colors #228

Merged
merged 6 commits into from
Aug 5, 2020
Merged

Fix not using custom chatbox's colors #228

merged 6 commits into from
Aug 5, 2020

Conversation

likeawindrammer
Copy link
Contributor

Fix #194

Moved set_text_color_dropdown() to after current_char is actually updated
with the selected char. Otherwise set_text_color_dropdown will try to
update the colors with either nothing as character, or with the previously
selected character.


Use get_chat() so it actually gets the name of the custom chatbox
instead of using the same name as the character.
Remove the extra "c" since get_chat_color it's already called
with this "c" in place.

Moved set_text_color_dropdown() to after current_char is actually updated
with the selected char. Otherwise set_text_color_dropdown will try to
update the colors with either nothing as character, or with the previously
selected character.


Use get_chat() so it actually gets the name of the custom chatbox
instead of using the same name as the character.
Remove the extra "c" since get_chat_color it's already called
with this "c" in place.
@likeawindrammer
Copy link
Contributor Author

Client crashes when trying to join DRO server for some reason, so I'll work more on this.

For some reason the client would work just fine
if set_text_color_dropdown() was called only once
after updating the character selected by the user.
But when joining a DRO server the client would crash
just before loading the music.
instead of trying to find the character folder on misc
@oldmud0
Copy link
Member

oldmud0 commented Aug 3, 2020

Why is this closed? Surely it fixes something.

Also remove the comment explaining the crash if set_text_color_dropdown
is removed from set_widgets
@likeawindrammer
Copy link
Contributor Author

Why is this closed? Surely it fixes something.

Because I found yet another bug related to this, so I wanted to work a little more on it before merging.

@oldmud0
Copy link
Member

oldmud0 commented Aug 3, 2020

Remember the draft button :^)

@likeawindrammer
Copy link
Contributor Author

Remember the draft button :^)

Isn't that an option only when you create the PR? To be honest I thought this was ready when I first open it. But now I'm sure 👍

@oldmud0
Copy link
Member

oldmud0 commented Aug 3, 2020

Isn't that an option only when you create the PR? To be honest I thought this was ready when I first open it. But now I'm sure

Originally you could only mark it as draft when creating the PR, but then they later added a button under the "reviewers" list to place it back into draft status.

@likeawindrammer likeawindrammer marked this pull request as draft August 3, 2020 20:32
@likeawindrammer likeawindrammer marked this pull request as ready for review August 3, 2020 20:32
@likeawindrammer
Copy link
Contributor Author

Isn't that an option only when you create the PR? To be honest I thought this was ready when I first open it. But now I'm sure

Originally you could only mark it as draft when creating the PR, but then they later added a button under the "reviewers" list to place it back into draft status.

Nice, thanks

@@ -435,7 +435,7 @@ QString AOApplication::get_tagged_stylesheet(QString target_tag, QString p_file)

QString AOApplication::get_chat_markdown(QString p_identifier, QString p_chat)
{
QString design_ini_path = get_base_path() + "misc/" + p_chat + "/config.ini";
QString design_ini_path = get_base_path() + "misc/" + get_chat(p_chat) + "/config.ini";
Copy link
Member

Choose a reason for hiding this comment

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

If the behavior changed in #230 is folded into get_chat per review, this might lead to unexpected behavior - in fact, one might begin to question what the purpose of the misc folder even is to begin with, if themes can take precedence over misc folders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, as far as this fix goes, get_chat_markdown was trying to access a folder named exactly as the character in misc. To me this is an obvious bug and should instead be reading the custom chatbox in misc. To me the order in which get_chat_markdown read ini files is something for another PR, #230 for example.

Copy link
Member

Choose a reason for hiding this comment

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

I'll allow this bugfix to pass, but having to manage the precedence orders of individual asset types like this is not setting a good precedent and will take various passes of what they call "un-fucking" later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, and like I said I can't wait to help you in what I can extinguishing the AO dumpster fire with the 2.9 branch.

@oldmud0 oldmud0 changed the base branch from master to 2.8 August 4, 2020 15:37
@oldmud0 oldmud0 added this to the 2.8.5 milestone Aug 4, 2020
@oldmud0 oldmud0 merged commit 0ce60d6 into 2.8 Aug 5, 2020
@mposs00 mposs00 deleted the windrammer/fix-194 branch August 16, 2020 22:03
lambdcalculus pushed a commit to lambdcalculus/AO2-Client that referenced this pull request Jun 2, 2024
…on-ooc-message-reception

Fix general client stutter when OOC messages are rendered
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.

[Bug 2.8] misc/folder/config.ini colors may be ignored
2 participants