D6856: Reset url in closeUrl()

This revision was automatically updated to reflect the committed changes. Closed by commit R306:1608c70efed4: Reset url in closeUrl() (authored by elvisangelaccio). REPOSITORY R306 KParts CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6856?vs=17278&id=17545 REVISION DETAIL https:/

D6856: Reset url in closeUrl()

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R306 KParts BRANCH reset-url-in-closeUrl REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

elvisangelaccio marked an inline comment as done. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

elvisangelaccio edited the summary of this revision. elvisangelaccio edited the test plan for this revision. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

elvisangelaccio updated this revision to Diff 17278. elvisangelaccio added a comment. - Drop TODO REPOSITORY R306 KParts CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6856?vs=17247&id=17278 BRANCH reset-url-in-closeUrl REVISION DETAIL https://phabricator.kde.org/D6856 AFFE

D6856: Reset url in closeUrl()

dfaure added a comment. Code looks good to me. INLINE COMMENTS > readonlypart.cpp:244 > > +// TODO KF6: make it non-virtual so that we can just call > +// closeUrlImpl() + setUrl(QUrl()) and get rid of m_closeUrlFromOpenUrl. I'm not sure about that TODO, it would be a horrendous porting tr

D6856: Reset url in closeUrl()

elvisangelaccio updated this revision to Diff 17247. elvisangelaccio added a comment. - Fix apidox REPOSITORY R306 KParts CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6856?vs=17246&id=17247 BRANCH reset-url-in-closeUrl REVISION DETAIL https://phabricator.kde.org/D6856 AFF

D6856: Reset url in closeUrl()

elvisangelaccio updated this revision to Diff 17246. elvisangelaccio edited the summary of this revision. elvisangelaccio added a comment. - Don't emit urlChanged() twice when calling openUrl(). REPOSITORY R306 KParts CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6856?vs=17149&id

D6856: Reset url in closeUrl()

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. 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

D6856: Reset url in closeUrl()

elvisangelaccio edited the summary of this revision. elvisangelaccio edited the test plan for this revision. REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

elvisangelaccio added a dependent revision: D6889: TestPart: add more test cases. REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

elvisangelaccio added a comment. @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: - openUrl(foo) - part loads url foo. - openUrl(bar) - op

D6856: Reset url in closeUrl()

elvisangelaccio updated this revision to Diff 17149. elvisangelaccio added a comment. - Use setUrl() to reset the URL. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6856?vs=17070&id=17149 BRANCH reset-url-in-closeUrl REVISION DETAIL https://phabricator.kde.org/D6856 AFFECTED

D6856: Reset url in closeUrl()

elvisangelaccio planned changes to this revision. elvisangelaccio added a comment. In https://phabricator.kde.org/D6856#128029, @cullmann wrote: > 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

D6856: Reset url in closeUrl()

cullmann added a comment. 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. REPOSITORY R306 KPa

D6856: Reset url in closeUrl()

elvisangelaccio added a reviewer: KDevelop. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop Cc: #frameworks

D6856: Reset url in closeUrl()

elvisangelaccio added reviewers: KTextEditor, Kate. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure, #ktexteditor, #kate Cc: #frameworks

D6856: Reset url in closeUrl()

dfaure added a comment. 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. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D6856 To: elvisangelaccio, dfaure Cc: #frame

D6856: Reset url in closeUrl()

elvisangelaccio created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Otherwise url() will return a possibly stale URL (for example, if loading failed). REPOSITORY R306 KParts BRANCH empty-url-af