If you press and hold F5 we try to reload multiple times, which isn't a nice idea, let's finish one before starting the next
Details
- Reviewers
- None
Diff Detail
- Repository
- R223 Okular
- Branch
- Applications/17.08
- Lint
No Linters Available - Unit
No Unit Test Coverage
Good catch, I stumbled upon this bug before. This even prevents segfaults on exit.
However, I think there's a potentially costly maintainability risk: When someone wants to change some code in slotDoFileDirty(), it's quite easy to forget resetting your new bool, causing all kinds of weird bugs down the line.
I thought about an alternative implementation using RAII without this problem and expanded the description a little bit. Please see D7595.
I shouldn't have really posted this for review, that's why i get for trying to get people to review my code 😄
Yes, the early return is a bit of a problem. But the slotDoFileDirty function has been majorly unchanged for years, and if someone breaks it, well it'll break and we'll fix it.
And while i told you on the other review that you don't need to write a RAII class, after all it's so easy that if you feel like it, sure do it.
SetBoolToFalseOnScopeExit aux(&m_areWeReloading);
should be less than 10 lines of code.
Yay for reviews finding problems before users do \o/
majorly unchanged for years, and if someone breaks it, well it'll break and we'll fix it.
If we can choose between a good and an improved solution, I'd say we should always pick the latter. Maybe I should have added a comment only instead of implementing it myself, but I wanted to check how it would work out and spare you some work. You have seen 384142 coming in only yesterday, please don't risk more crashes unnecessarily.
SetBoolToFalseOnScopeExit [...] should be less than 10 lines of code.
That's both harder to understand and longer than the additional 0 lines of code for the RAII-mutex solution, as well as less compact in terms of the number of separate code fragments.
I'm all for fixing the original issue, but let's do it properly.