symbols are now put into a standard item model
Details
Details
Diff Detail
Diff Detail
- Repository
- R40 Kate
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Here is a initial try to convert the symbols storage to a model.
The only thing that ATM is not integrated is the optional "flat" display mode.
Is that really needed? One could emulate it with yet-an-other proxy model or by telling the server if one wants to have a flat or non-flat model.
The filter line edit works now like for the project plugin with recursive filtering.
Comment Actions
Btw., the server part could cache the latest X requested models to provide them faster again to the views if one switches between different views back and forth.
With the std::shared_ptr approach no leaks should happen.
And it is save that multiple views use the same model (by design), the don't alter it.
Comment Actions
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), 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.
Comment Actions
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.
Comment Actions
;=) Next try.
This is a lot more local: just swapped the TreeWidget with a TreeView and do local model construction.
The 4 issues you mentioned should be fixed, too.
I am not 100% sure if the sorting is handled correctly, if you have some input there, that would be nice ;=)
Comment Actions
Ah, one other quick question: The "detail" field is at the moment not parsed and filled at all, is that an oversight?
Comment Actions
The detail field not getting parsed probably started out intentionally and then turned into an oversight :-(
Regarding the sorting, it seems the following is needed (in suitable place);
if (m_sortOn->isChecked()) {
m_symbols->setSortingEnabled(true);
m_symbols->sortByColumn(0);
} else {
m_symbols->sortByColumn(-1);
}Otherwise, it looks/works fine now!
Comment Actions
Ok, will land this then.
Should I add the missing bit for the parsing? Is just a line ;=)