D6856: Reset url in closeUrl()

2017-08-02 Thread Elvis Angelaccio 
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=17545

REVISION DETAIL
  https://phabricator.kde.org/D6856

AFFECTED FILES
  autotests/parttest.cpp
  autotests/parttest.h
  src/readonlypart.cpp
  src/readonlypart.h
  src/readonlypart_p.h

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-27 Thread David Faure 
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()

2017-07-27 Thread Elvis Angelaccio 
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()

2017-07-27 Thread Elvis Angelaccio 
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()

2017-07-27 Thread Elvis Angelaccio 
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=17278

BRANCH
  reset-url-in-closeUrl

REVISION DETAIL
  https://phabricator.kde.org/D6856

AFFECTED FILES
  autotests/parttest.cpp
  autotests/parttest.h
  src/readonlypart.cpp
  src/readonlypart.h
  src/readonlypart_p.h

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-26 Thread David Faure 
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 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.

REPOSITORY
  R306 KParts

REVISION DETAIL
  https://phabricator.kde.org/D6856

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-26 Thread Elvis Angelaccio 
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=17247

BRANCH
  reset-url-in-closeUrl

REVISION DETAIL
  https://phabricator.kde.org/D6856

AFFECTED FILES
  autotests/parttest.cpp
  autotests/parttest.h
  src/readonlypart.cpp
  src/readonlypart.h
  src/readonlypart_p.h

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-26 Thread Elvis Angelaccio 
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=17246

BRANCH
  reset-url-in-closeUrl

REVISION DETAIL
  https://phabricator.kde.org/D6856

AFFECTED FILES
  autotests/parttest.cpp
  autotests/parttest.h
  src/readonlypart.cpp
  src/readonlypart_p.h

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-24 Thread David Faure 
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 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).

REVISION DETAIL
  https://phabricator.kde.org/D6856

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-24 Thread Elvis Angelaccio 
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()

2017-07-24 Thread Elvis Angelaccio 
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()

2017-07-24 Thread Elvis Angelaccio 
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)
  - openUrl() calls closeUrl()
  - part fails to load url bar.
  
  In this case we only get one urlChanged() signal since we could not open the 
new url.

REVISION DETAIL
  https://phabricator.kde.org/D6856

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-24 Thread Elvis Angelaccio 
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=17149

BRANCH
  reset-url-in-closeUrl

REVISION DETAIL
  https://phabricator.kde.org/D6856

AFFECTED FILES
  src/readonlypart.cpp

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-24 Thread Elvis Angelaccio 
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 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.

REPOSITORY
  R306 KParts

REVISION DETAIL
  https://phabricator.kde.org/D6856

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-23 Thread Christoph Cullmann 
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 KParts

REVISION DETAIL
  https://phabricator.kde.org/D6856

To: elvisangelaccio, dfaure, #ktexteditor, #kate, #kdevelop
Cc: cullmann, #frameworks

D6856: Reset url in closeUrl()

2017-07-23 Thread Elvis Angelaccio 
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()

2017-07-23 Thread Elvis Angelaccio 
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()

2017-07-23 Thread David Faure 
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: #frameworks

D6856: Reset url in closeUrl()

2017-07-23 Thread Elvis Angelaccio 
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-after-closeurl

REVISION DETAIL
  https://phabricator.kde.org/D6856

AFFECTED FILES
  autotests/parttest.cpp
  autotests/parttest.h
  src/readonlypart.cpp

To: elvisangelaccio, dfaure
Cc: #frameworks