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

can not sell vehicle #676

Open
ghost opened this issue May 26, 2020 · 3 comments
Open

can not sell vehicle #676

ghost opened this issue May 26, 2020 · 3 comments

Comments

@ghost
Copy link

ghost commented May 26, 2020

Expected behaviour

let the confirmation dialog come up as in #634

Actual behaviour

fn_sellGarage errors out as lbCurSel returns -1 cause the dialog was closed too early in impound.hpp

Steps to reproduce the behaviour

try to sell a vehicle from a garage

RPT and/or extDB3 logs (if applicable)

not applicable

Mission version:
Launcher version:
Game version:
Branch: current master

I'm really sorry to write this again - but just another instance of "not tested before pushing public". In #634 impound.hpp was changed so that the dialog is closed before fn_sellGarage is called - but this causes lbCurSel 2802 to return -1 instead of the index. It's basically a 1-to-1 copy from the unimpound function directly above. And obviously this was not tested after implementing - otherwise @blackfisch should had noticed that for what ever reason when clicking on sell the script fails with the STR_Global_NoSelection hint. A simple debug like
_curIndex = lbCurSel 2802; diag_log format ["%1", _curIndex];
right at the very top of fn_sellGarage show this error. This is not about "master branch is not recommended" but rather "devs just think and believe their changes do what they intent and push without any testing" - this is a serious process issue - not lack of coding skills in the SQF script language.

@ghost ghost mentioned this issue May 26, 2020
1 task
@blackfisch
Copy link
Contributor

You can trust me, I tested this.
This seems like an issue that can come up, not has to come up. Arma can be weird with that kind of issues, working on one machine but not working on others.

@blackfisch
Copy link
Contributor

adding to that; as described to you in #673 when adding/changing functionality, we test for upcoming errors. a full "contextual" test is performed before release. Test-driven development is not really applicable to Arma 3 - you'd have to take hours of testing for literally a 10 line change. That's simply not realistic.
We appreciate your help, but that doesn't mean you need to discredit anyone else working on the project because some errors are still on the WIP-branch of the project. That's why it's WIP and not recommended to use for productivity atm.

@ghost
Copy link
Author

ghost commented May 26, 2020

I don't see where there could be any difference unless BI changed how lbCurSel works in the time between your PR got merged and my test - as I doubt that it's a race condition like when two threads access the same peice of memory at the same time. Sure it's maybe unexpected that the selection index gets lost when the dialog is closed, but it's not uncommon as the selection index should belong to the instance of the dialog - which get's cleaned up when the dialog is cleaned up- proper design of the UI code. In other languages like Java when try to access an index of a list of an already disposed frame you get a very long stacktrace as the objects you try to access no longer exists. TLDR: Try to access an index of something that no longer exists and was already cleaned up doesn'T make sense in the first place. The -1 or a more severe errror was to be expected.

@ghost ghost mentioned this issue May 28, 2020
1 task
@ghost ghost closed this as completed May 29, 2020
@BoGuu BoGuu reopened this May 29, 2020
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

No branches or pull requests

2 participants