Emit urlChanged() only if we actually open the URL
AbandonedPublic

Authored by elvisangelaccio on Jul 24 2017, 9:37 PM.

Details

Reviewers
dfaure
Group Reviewers
Kate
KDevelop
Summary

Otherwise url() returns a wrong value if the part could not load the URL.

Test Plan

Unit tests in D6889

Diff Detail

Branch
properly-emit-urlChanged
Lint
No Linters Available
Unit
No Unit Test Coverage
dfaure requested changes to this revision.Jul 24 2017, 10:59 PM

No.

The code inside openFile() expects url() to return the correct (target) URL. This is very important (e.g. to display an error message if it fails to load)

So this also means my previous comment in D6856 won't work.
Instead, we need

oldUrl --> newUrl --> QUrl() if it fails to load.
This revision now requires changes to proceed.Jul 24 2017, 10:59 PM

Actually I am not sure if it is at all desirable to not "keep" the url even on failure.
E.g. it allows to try "reload" easily and people might just rely on that "bug".

Actually I am not sure if it is at all desirable to not "keep" the url even on failure.
E.g. it allows to try "reload" easily and people might just rely on that "bug".

I would be ok with discarding this change (if D6856 goes in, we would have a way to explicitly reset the url, if necessary), but then we need to update the url() apidox because right now it claims: "Returns the URL currently opened in this part."

Yeah, "opened" or "being opened", if you want to nitpick ;-)

elvisangelaccio abandoned this revision.Jul 26 2017, 5:48 PM

Apidox fix is in D6856.