Re: riched20: new selection invalidation logic
Monday, August 7, 2006, 9:27:16 AM, Krzysztof Foltman wrote:
ChangeLog: * New, clean, simple selection repaint logic - should fix all outstanding refresh issues (and bug#5882)
Krzysztof
I think this patch takes number of asserts per line to the highest level I've ever seen. Could you explain why you adding asserts everywhere (only 11 in this patch alone!!!)? Instead of using proper error handling? For the past year or more that was the main source of user problems. And if you could how many installers have been broken with this technique, you will probably account for 90% of all the installers that otherwise would work on Wine. Have you even tried your code on purposely broken rtf? How does it handle it? Vitaliy Margolen
Vitaliy Margolen wrote:
I think this patch takes number of asserts per line to the highest level I've ever seen. Could you explain why you adding asserts everywhere (only 11 in this patch alone!!!)? Instead of using proper error handling?
Well, that's the good question. The thing is, those conditions should never happen, and if they do happen, then something is really wrong somewhere with the code and a crash may happen anyway. I think it's better to get an assertion failure early than a random crash when it's too late to diagnose anything. And one can always comment the asserts out. But there are valid reasons why those asserts should be avoided. It's just about short term vs long term benefits. Maybe an option to disable those asserts would be a good idea so that people who just need to get the job done aren't messed with - but then we don't get half the bug reports we get now. As for the enormous (or even insane) amount of asserts - there's no such thing ;) Those asserts are pretty "paranoid-level" - chances of any of them happening is really, really low - and that's totally different to my previous repaint patch which had one assertion I predicted may be risky (which proved something is wrong with repaint, so it kinda served the purpose). The good solution would be a decent regression test, but it seems to be pretty difficult and/or tedious (simulating real-life conditions for testing repainting is hard because of relative timing/order of window messages).
For the past year or more that was the main source of user problems. And if you could how many installers have been broken with this technique, you will probably account for 90% of all the installers that otherwise would work on Wine.
Have you even tried your code on purposely broken rtf? How does it handle it?
Those asserts are relatively independent on text input, and more on user interaction with the editor, which, as I already said, is hard to simulate and test (that's why I'm kind of shifting responsibility to end users: I may do as much monkey tests as you want, still I may get into some ruts of usage pattern). Anyway, the issue is not easy to solve, so I'd be grateful for any ideas that may help in solving this important issue. Krzysztof
Monday, August 7, 2006, 10:33:02 AM, Krzysztof Foltman wrote:
Vitaliy Margolen wrote:
I think this patch takes number of asserts per line to the highest level I've ever seen. Could you explain why you adding asserts everywhere (only 11 in this patch alone!!!)? Instead of using proper error handling?
Well, that's the good question. The thing is, those conditions should never happen, and if they do happen, then something is really wrong somewhere with the code and a crash may happen anyway. I think it's better to get an assertion failure early than a random crash when it's too late to diagnose anything. And one can always comment the asserts out. Of course it's good when you don't affect anything else around your code. If you can make some exception handlers that will catch that assert and print nice error instead of RTF that would be awesome. And not a single person will be upset. Users will have their functioning installers and you will have your error reports.
But there are valid reasons why those asserts should be avoided. It's just about short term vs long term benefits. Correct. 2 years ought to be enough?
For the past year or more that was the main source of user problems. And if you could how many installers have been broken with this technique, you will probably account for 90% of all the installers that otherwise would work on Wine.
Have you even tried your code on purposely broken rtf? How does it handle it?
Those asserts are relatively independent on text input, and more on user interaction with the editor, which, as I already said, is hard to simulate and test (that's why I'm kind of shifting responsibility to end users: I may do as much monkey tests as you want, still I may get into some ruts of usage pattern). I don't see how that can be if users report crashes immediately when installer tries to show licence. Or when user tries to scroll text to the bottom so "Next" button will become enabled.
Anyway, the issue is not easy to solve, so I'd be grateful for any ideas that may help in solving this important issue. _TRY { call riched20 } _EXCEPT(handler) _ENDTRY
Seems like easy to me. Vitaliy.
Vitaliy Margolen wrote:
I don't see how that can be if users report crashes immediately when installer tries to show licence. Or when user tries to scroll text to the bottom so "Next" button will become enabled.
It doesn't happen on my machine on installers I've tried (including the infamous NSIS), so it's not because of lack of testing at all. In fact, I still don't know why it happens on one machine and not another.
Anyway, the issue is not easy to solve, so I'd be grateful for any ideas that may help in solving this important issue. _TRY { call riched20 } _EXCEPT(handler) _ENDTRY Seems like easy to me.
How does that solve the improper operation vs lack of bug reports issue? Do we want to have buggy richedit forever? Unless you put some mail sending code in the exception handler. But that qualifies as spyware and shouldn't happen. Krzysztof
Vitaliy Margolen wrote:
upset. Users will have their functioning installers and you will have your error reports.
I will have my error reports? How exactly will I get them? Does average user (or Wine) send us err: error reports? If he/she usually had, this would be a non-issue, and asserts could be replaced with if (!condition) ERR(...). Krzysztof
Have you even tried your code on purposely broken rtf? How does it handle it?
Broken rtf can only possibly break the RTF reader code, although it has not been happening for quite a long time. And the asserts are there to report Wine riched20 developer's errors, not the app developers'. -- Ph.
Monday, August 7, 2006, 1:48:49 PM, Phil Krylov wrote:
Have you even tried your code on purposely broken rtf? How does it handle it?
Broken rtf can only possibly break the RTF reader code, although it has not been happening for quite a long time. And the asserts are there to report Wine riched20 developer's errors, not the app developers'.
Then we should clean it up and make it not crash. Or that's remove all FIXMEs and replace them with asserts. Will that help? Vitaliy
participants (3)
-
Krzysztof Foltman -
Phil Krylov -
Vitaliy Margolen