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

0009611: refactor GUIGetChatboxLayout, and add the CVar argument as advertised on Wiki #254

Merged
merged 3 commits into from Aug 27, 2018

Conversation

Projects
3 participants
@patrikjuvonen
Collaborator

patrikjuvonen commented Jul 23, 2018

Mantis Bug Tracker issue:
9611

Summary:

  • Refactored GUIGetChatboxLayout in CLuaGUIDefs;
  • Added the CVar argument as advertised on Wiki (been missing all this time);
  • Pretty small and simple fix, easy to test.

Tested with:

local cvars = {
	123,
	"nonsense_cvar",
	"chat_font",
	"chat_lines",
	"chat_color",
	"chat_text_color",
	"chat_input_color",
	"chat_input_prefix_color",
	"chat_input_text_color",
	"chat_scale",
	"chat_position_offset_x",
	"chat_position_offset_y",
	"chat_position_horizontal",
	"chat_position_vertical",
	"chat_text_alignment",
	"chat_width",
	"chat_css_style_text",
	"chat_css_style_background",
	"chat_line_life",
	"chat_line_fade_out",
	"chat_use_cegui",
	"text_scale",
	false
}

for i, cvar in ipairs(cvars) do
	local result = getChatboxLayout(cvar)
	outputConsole("cvar: [" .. cvar .. "]; result: [" .. inspect(result) .. "]; type: [" .. type(result) .. "]")
end

local result = getChatboxLayout()
outputConsole("all cvars (no args); result: [" .. inspect(result) .. "]; type: [" .. type(result) .. "]")

for cvar, result in pairs(result) do
	outputConsole("cvar (from all cvars table): [" .. cvar .. "]; result: [" .. inspect(result) .. "]; type: [" .. type(result) .. "]")
end

patrikjuvonen added some commits Jul 23, 2018

@patrikjuvonen patrikjuvonen changed the title from 0009611: refactor GUIGetChatboxLayout, and add the CVar argument as advertised on Wiki to WIP 0009611: refactor GUIGetChatboxLayout, and add the CVar argument as advertised on Wiki Jul 24, 2018

@Pirulax

This comment has been minimized.

Show comment
Hide comment
@Pirulax

Pirulax Jul 24, 2018

Contributor

So you added some new cvars too?

Contributor

Pirulax commented Jul 24, 2018

So you added some new cvars too?

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 24, 2018

Member

i think returning table of numbers is fine as it's compatible with the full table format @patrikjuvonen

Member

qaisjp commented Jul 24, 2018

i think returning table of numbers is fine as it's compatible with the full table format @patrikjuvonen

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 24, 2018

Collaborator

@Pirulax No it's using the same as it did before.
@qaisjp Wiki advertises it returns 4 numbers for a color and 2 numbers for a scale. I think a table is unnecessary, and isn't consistent with getVehicleColor etc.

Collaborator

patrikjuvonen commented Jul 24, 2018

@Pirulax No it's using the same as it did before.
@qaisjp Wiki advertises it returns 4 numbers for a color and 2 numbers for a scale. I think a table is unnecessary, and isn't consistent with getVehicleColor etc.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 24, 2018

Member

we can change the wiki. In fact I'm not sure if this feature is necessary anymore (although it is a nice refactor so it'd be nice to keep that!)

Member

qaisjp commented Jul 24, 2018

we can change the wiki. In fact I'm not sure if this feature is necessary anymore (although it is a nice refactor so it'd be nice to keep that!)

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 24, 2018

Collaborator

Ok, let's keep it as a table.

Collaborator

patrikjuvonen commented Jul 24, 2018

Ok, let's keep it as a table.

@patrikjuvonen patrikjuvonen changed the title from WIP 0009611: refactor GUIGetChatboxLayout, and add the CVar argument as advertised on Wiki to 0009611: refactor GUIGetChatboxLayout, and add the CVar argument as advertised on Wiki Jul 24, 2018

@Pirulax

This comment has been minimized.

Show comment
Hide comment
@Pirulax

Pirulax Jul 24, 2018

Contributor

If you do, then the wiki is missing some cvars.
Should i add them?
Or some of the cvars in ur table aren't real cvars?(other than the "nosense_cvar")

Contributor

Pirulax commented Jul 24, 2018

If you do, then the wiki is missing some cvars.
Should i add them?
Or some of the cvars in ur table aren't real cvars?(other than the "nosense_cvar")

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 24, 2018

Collaborator

@Pirulax getChatboxLayout is supposed to return chatbox related CVars, nothing else.
The test contains a nonsense cvar to make sure it returns false when the CVar given is invalid.

Collaborator

patrikjuvonen commented Jul 24, 2018

@Pirulax getChatboxLayout is supposed to return chatbox related CVars, nothing else.
The test contains a nonsense cvar to make sure it returns false when the CVar given is invalid.

@Pirulax

This comment has been minimized.

Show comment
Hide comment
@Pirulax

Pirulax Jul 24, 2018

Contributor

image
These are the CVars that wiki lists.

Contributor

Pirulax commented Jul 24, 2018

image
These are the CVars that wiki lists.

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Jul 24, 2018

Collaborator

Ok, looks like the Wiki article is not up to date. Needs to be updated.

Collaborator

patrikjuvonen commented Jul 24, 2018

Ok, looks like the Wiki article is not up to date. Needs to be updated.

@patrikjuvonen patrikjuvonen added this to In progress in release/v1.5.6 via automation Jul 24, 2018

@patrikjuvonen patrikjuvonen added this to the 1.5.6 milestone Jul 24, 2018

release/v1.5.6 automation moved this from In progress to Ready Aug 26, 2018

@qaisjp

qaisjp approved these changes Aug 26, 2018

@qaisjp qaisjp merged commit e268a09 into multitheftauto:master Aug 27, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

release/v1.5.6 automation moved this from Ready to Done Aug 27, 2018

@patrikjuvonen patrikjuvonen deleted the patrikjuvonen:issue-9611 branch Aug 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment