D22592: port symbols view to model/view concept

[Christoph Cullmann]

cullmann planned changes to this revision.
cullmann added a comment.


  In D22592#498923 <https://phabricator.kde.org/D22592#498923>, @mnauwelaerts wrote:
  
  > Thanks for the new feature!  However, a few regressions and concerns:
  >
  > - the "show details" action is no longer enabled/disabled depending on whether useful detail info is available
  > - the detail is now in a separate column; if a server decides to present e.g. argument info there, it does not look that good in a separate column with not-so-useful white space in between (others thing probably also not either).  While a bit hack-ish, that's why it was previously merged with the "main text".
  > - if the symbol view tree is (partially) collapsed, and then the item tracking kicks in to sync with cursor position, the tree is expanded again, rather than showing topmost relevant (visible) item. Previously, it was kept collapsed, as presumably so done by user for a reason.
  > - the flat-list option is gone as well as already mentioned (whether or not it may be deemed useful)
  >
  >   There are good arguments for moving to a model/view where a single treewidget is now treeview/model combination, such as to support filtering as added here.  However, most of the other issues seem due to moving part of the "presentation layer" (the model part) to the server layer.  The latter now returns a (wrapped) QStandardItemModel, which is (essentially) a not so descriptive variant type.  So it requires inspection of the implementation to see what is happening, and ties implementation in one (view) part to implementation elsewhere.  As a result, deciding what/where/how to do with flat (or not) or where/how to present "detail info" is less transparent and plain-and-simple.  Additional parameters could indeed be passed down to the server call and that would effectively work, but also clearly such parameters are not relevant to protocol layer (= server responsibility).  So it would all work, but become blurry as to what server part actually represents and what part is doing what, since it would then do do some protocol level stuff/call and then some <not-always-so-clear> ...  Maybe the detail thingy or the flat list is not so important or useful (now?), but then it feels a bit like dropping some functionality because implementation does not really handle it well/nicely (whereas conversely implementation should serve functionality/features).  And more relevant, other more important presentation choices may arise (such as perhaps already the detail representation now) and would then become more complicated and less flexible to handle (e.g. require not so plain-and-simple proxy model or passing stuff back and forth to server).  Also note that the server part has acquired more dependencies (a.o. QtGui), which even more reduces any chance/possibility of splitting out things for use by 3rd party (as mentioned in D22595 <https://phabricator.kde.org/D22595>), and for which reason the (server) dependencies have been kept minimum so far.
  >
  >   So, in this view, it seems/feels like we could have our cake and eat it, that is, avoid those (future) issues and still have the new filter option (and icon on demand etc) by having a server layer as it was and then using treeview/model combination on the (symbol)view level.  Likewise, the latter view part is  also in a good position to cache server call results and can simply forego submitting to server if it finds it cached (in some map or so).  It already knows about text changes (by signal) and can invalidate (local) cache entry at that time.
  
  
  Thanks for the review ;=)
  
  I agree on most points, without implementing a own model, which is error prone, stuff like dynamic show details or flat-list need e.g. own item delegates and yet-an-other extra filter model.
  Given that will make the code base twice as large as with your initial implemetation, I will backtrack here and just create the model in the view as needed, like you did before, then the only difference will be the use of the QTreeView and the ability to filter.
  I will take care of the other regressions ;)
  
  Then you can take a look again, one can still at a later point again decide if one wants to lower the model to a different layer.

REPOSITORY
  R40 Kate

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

To: cullmann, dhaumann, mnauwelaerts
Cc: kwrite-devel, domson, michaelh, ngraham, demsking, cullmann, sars, dhaumann
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20190721/b53ae3df/attachment-0001.html>