| dfaure |
| KTextEditor | |
| Kate | |
| KDevelop |
closeUrl() is supposed to close the currently open url, so we should not
leave the old url() around if someone explicitly calls this method.
To avoid urlChanged() being emitted twice if we call openUrl(), we use a
special boolean set in openUrl(). We need this boolean because openUrl()
is both public and virtual and we need to make sure to call the
openUrl() re-implemented by subclasses.
New test case passes.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
It seems to make sense, but since it's been that way for so long, I'd like confirmation from at least Kate / KDevelop that this doesn't break anything.
Hi, I see the issue that the url is changed without calling setUrl. That doesn't emit urlChanged() that way.
On the other side, if setUrl would be used, you will get the urlChanged twice in openUrl which might lead to not-wanted side-effects.
Good catch, this patch is wrong but the fact that the part has an URL set even if it failed to open that URL is a bug that should be fixed, imho.
I'll try another approach.
@cullmann After thinking more about it, the URL does change twice (old url -> QUrl(), QUrl() -> new url), so it should be expected to get two urlChanged() signals.
Consider this scenario:
In this case we only get one urlChanged() signal since we could not open the new url.
The URL changes twice because you make it change twice ;)
But that can lead to the application updating stuff twice, which doesn't seem very optimal. (actions getting disabled and then re-enabled, title bar flickering, stuff like that).
Ideally it should change only once, either from oldURL to newURL, or from oldURL to QUrl() if loading failed.
This might require a special bool checked by closeUrl(), since we can't just split closeUrl (because it's public *and* virtual - bad design practice - I was young).
(if it wasn't virtual, then moving the code into a closeUrlImpl and having the public closeUrl() do closeUrlImpl()+setUrl(QUrl()) would be the best solution, but we can't do that since openUrl has to call the virtual closeUrl, not just closeUrlImpl. So I can't think of another solution than a bool member set by openUrl and checked by closeUrl to skip the setUrl(QUrl()) until after we try to open the new URL).
Code looks good to me.
| src/readonlypart.cpp | ||
|---|---|---|
| 244 | I'm not sure about that TODO, it would be a horrendous porting trap (closeUrl not called anymore, no way to detect it unless override was used) I was commenting on the problem with this design, but I don't think we should actually change it, the bool works. | |