D11278: [KateCompletionWidget] Create configuration interface on demand

[Kai Uwe Broulik]

broulik abandoned this revision.

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: hein, martinkostolny, cullmann, kfunk, #frameworks, michaelh, kevinapavew, 
ngraham, bruns, demsking, sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Christoph Cullmann]

cullmann requested changes to this revision.
cullmann added a comment.
This revision now requires changes to proceed.


  If we want that, one needs to take care of the config loading/initial setup 
of the m_presentationModel in a different way than relying on the config widget 
to do it.

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: hein, martinkostolny, cullmann, kfunk, #frameworks, michaelh, kevinapavew, 
ngraham, demsking, sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Christoph Cullmann]

cullmann reopened this revision.
cullmann added a comment.
This revision is now accepted and ready to land.


  See:
  
  Git commit 7f3d9e774129618dfb9fd871d5d5c8fbb66b4d9a 
 by 
Christoph Cullmann.
  Committed on 18/03/2018 at 12:33.
  Pushed by cullmann into branch 'master'.
  
  Revert "[KateCompletionWidget] Create configuration interface on demand"
  
  This reverts commit 92e21fb03b7fd01eab6fd6f4a116b849cb93ef9e 
.
  
  KateCompletionConfig construction seems to have some side-effects that are 
necessary
  (e.g. initial config loading)
  
  M  +1-5src/completion/katecompletionwidget.cpp
  
  https://commits.kde.org/ktexteditor/7f3d9e774129618dfb9fd871d5d5c8fbb66b4d9a

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: hein, martinkostolny, cullmann, kfunk, #frameworks, michaelh, kevinapavew, 
ngraham, demsking, sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Christoph Cullmann]

cullmann added a comment.


  Perhaps one has not thought about the side-effects of the construction of the 
KateCompletionConfig.
  It looks like it does a readConfig and that triggers applyInternal() which in 
return will configure the m_presentationModel.
  I will revert this commit for the time being.

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: hein, martinkostolny, cullmann, kfunk, #frameworks, michaelh, kevinapavew, 
ngraham, demsking, sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Eike Hein]

hein added a comment.


  A backtrace for the crash @martinkostolny mentioned can be found here: 
https://bugs.kde.org/show_bug.cgi?id=391955

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: hein, martinkostolny, cullmann, kfunk, #frameworks, michaelh, kevinapavew, 
ngraham, demsking, sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Martin Kostolný]

martinkostolny added a comment.


  Hi! After this commit I don't see a completion dropdown anymore. And after it 
should be opened an one presses backspace, kate or kwrite crashes. Simple 
example:
  
  1. open empty Kate
  2. write "", enter, "", backspace
  
  I will provide backtrace on demand :). I'm running on kdesrc-build 
environment, therefore frameworks and KDE applications are built from git. I 
have stable Qt version 5.10.1.

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: martinkostolny, cullmann, kfunk, #frameworks, michaelh, kevinapavew, 
ngraham, demsking, sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:92e21fb03b7f: [KateCompletionWidget] Create configuration 
interface on demand (authored by broulik).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11278?vs=29382=29400

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

AFFECTED FILES
  src/completion/katecompletionwidget.cpp

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: cullmann, kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, 
sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.


  I think it is ok that way, too ;=)

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk, cullmann
Cc: cullmann, kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, 
sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Kevin Funk]

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  In D11278#224484 , @kfunk wrote:
  
  > Looks like you don't need the member at all? Otherwise late-init wouldn't 
work this way.
  >
  > `m_configWidget` seems only used in `showConfig()`. Let's remove the member 
altogether?
  
  
  Ah, disregard my comment.  If `showConfig()` is invoked multiple times then 
my approach would be slower.

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann, kfunk
Cc: kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Kevin Funk]

kfunk added a comment.


  Looks like you don't need the member at all? Otherwise late-init wouldn't 
work this way.
  
  `m_configWidget` seems only used in `showConfig()`. Let's remove the member 
altogether?

REPOSITORY
  R39 KTextEditor

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

To: broulik, #ktexteditor, dhaumann
Cc: kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: KTextEditor, dhaumann.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.
broulik requested review of this revision.

REVISION SUMMARY
  No need to create it immediately. Saves some cycles on startup.

TEST PLAN
  Compiles. I didn't find a way to actually trigger the config UI, though

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/completion/katecompletionwidget.cpp

To: broulik, #ktexteditor, dhaumann
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann