It makes no sense to have them not there or to treat them special
- Disable menu options dependent from current setting
Details
Details
- Reviewers
sars - Group Reviewers
Kate - Commits
- R40:3e6261a10bdd: SymbolView: Add expand and parameter options to popup menu
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
I like to request a "go" to remove the config page from this plugin.
I see no advantage to have this page but it will make much more work to keep it working when the menu is created by each parser.
The current menu settings can be saved in the new "actionTriggered()" function.
If you prefer can the code kept so far but disabled/out-commented for maybe later use with a differend purpose.
Comment Actions
I think it is ok to add these to the popup.
- The changes in the settings page is permanent, while the changes in the popup-menu is temporary -> I don't want to get rid of the settings page.
- actionTriggered() is now almost like the removed slotRefreshSymbols() and besides all parsers are already checking the expand on status... do we need actionTriggered()?
- I would like to have expandOn to behave like a default value and if I manually toggle the tree it should stay that way also if the document is changed. At the moment it expands/collapses all the tree-nodes.
This would be a change from the current behavior. Changing the expandOn should of course apply the change immediately.
(4) I did not want the name-change ;)
Thanks,
Kåre
Comment Actions
Do you miss the update in D14518 too?
The changes in the settings page is permanent, while the changes in the popup-menu is temporary -> I don't want to get rid of the settings page.
As said, it can/will be saved automatic. Then it is permanent and that page useless.
do we need actionTriggered()?
Yes. Doing so in parseSymbols() is unneeded often
if I manually toggle the tree it should stay that way also if the document is changed.
That's how it will be work
As said, I want for each parser/document type independent saved settings
At the moment it expands/collapses all the tree-nodes.
This is how it works. All or nothing. There is no other possibility
I did not want the name-change ;)
Anyone else out there who like my patch D14611?
Regards, Lothar
Comment Actions
That would work differently from the rest of KTextEditor options, which would not be a good thing.
do we need actionTriggered()?
Yes. Doing so in parseSymbols() is unneeded often
But we are not there yet. Now we have bool+action -> action change. The actionTriggered() seems to be part of a change that you have in mind to do but have not presented yet.
if I manually toggle the tree it should stay that way also if the document is changed.
That's how it will be work
Nice :)
As said, I want for each parser/document type independent saved settings
If we would save the state for each plugin and type separately, this m_expandOn action is not needed at all... And again saving the settings directly from the popup would be different from the rest of Kate where popup menus only are temporary changes.
At the moment it expands/collapses all the tree-nodes.
This is how it works. All or nothing. There is no other possibility
I did not want the name-change ;)
Anyone else out there who like my patch D14611?
Regards,
Kåre
Comment Actions
As said, it can/will be saved automatic. Then it is permanent and that page useless.
That would work differently from the rest of KTextEditor options, which would not be a good thing.
Have you a couple of examples which options do you mean?
Now we have bool+action -> action change.
Which bool is left? They are all gone now(?)
do we need actionTriggered()?
The actionTriggered() seems to be part of a change that you have in mind to do but have not presented yet.
Well, these actionTriggered() may later moved into the parser class. Right now does it make anyway a good job, it is something like a sneak preview (or workaround) without the need for too much changes elsewhere.
I have the feeling you have not tried it, only looked at the diff. Whithout these menu morhp would there an option available which cause no effect when not in tree mode.
Normally are always small patches desired whereas big patches cause a lot grumble at maintainer side. So I try my best to do it slow. I don't like to go fast forward in my own pace until I have a nice working solution which make me happy but no chance to be accepted from your side. As we can see, you have already now, with all these small changes, so much to grumble.
As said, I want for each parser/document type independent saved settings
If we would save the state for each plugin and type separately, this m_expandOn action is not needed at all
Why that? Without m_expandOn it will not expanded...
if I manually toggle the tree it should stay that way also if the document is changed.
...could it be you wish a complete different behaviour? Now, click on some tree node to expand it, has absolutly no further action in respond. Change to an differend document and back will lost that setting. That will also the case after may patches. If you want to save the exact tree state to each document, make suggestion how to achive this.
It may nice or may unneded effort, but is in any case a complete differnd task.
saving the settings directly from the popup would be different from the rest of Kate where popup menus only are temporary changes.
Which may a wrong decision or a bug :-)
To achive that (ugly) behaviour we can add a "Save this settings as default" action (in a shorter name) to the popup menu or a "Save always changes in this menu" option.
Keep in mind that there are some things stored to each document, like line number or other settings without user action. We can do it for this plugin too, but I'm not a big fan of so much stroring bloat to each file I have edit at sometime.
Regards, Lothar
Comment Actions
Except for the actionTriggered() function I'm ready to accept this patch.
Examples of options that are not saved when changed from menus: View -> font size changes, View -> Word wrap -> *, View -> Border -> *, Tools -> Spelling -> *,....
It is a design decision to have permanent changes in the settings pages and temporary document specific changes in the menus.
| addons/symbolviewer/plugin_katesymbolviewer.cpp | ||
|---|---|---|
| 269 | What I meant was that you could move this line to parseSymbols() and not add a new "wrapper" function with only one extra line of code... If you absolutely want to save the one setEnabled() for the timer-based document update, please use a more descriptive name like displayOptionChanged() or something similar. | |
Comment Actions
I'm ready to accept this patch.
Wow, good news, thanks!
But now I'm curious which part of my text was so persuasive.
Except for the actionTriggered() function
Woops! That start was not so nice. Next time please add a hint for more information below ;-)
What I meant was that you could move this line to parseSymbols() and not add a new "wrapper" function with only one extra line of code...
I found a second line I have missed :-)
...If you absolutely want to save the one setEnabled() for the timer-based document update,
As said I want to save there the settings too. And that would then be unneeded often done.
Independently of that would it look not right to me that menu morph to do in parseSymbols().
please use a more descriptive name like displayOptionChanged() or something similar.
With pleasure. Good suggestion!
Examples of options that are not saved when changed from menus: View -> font size changes, View -> Word wrap -> *, View -> Border -> *, Tools -> Spelling -> *,....
Thanks for the list.
"Word wrap" and "Spelling" are both annoying. I have so often to fumble unneeded with that.
It is a design decision to have permanent changes in the settings pages and temporary document specific changes in the menus.
Almost good, and right. But! ;-)
The wrong parts there are
- that these "permanent changes in the settings page" should only be "presets" when you choose an unseen document or create a new one (so far working) and
- that the changes done in the menu should be saved permanent in relation to the current document (can't give a test case, but I think it's not working or at least buggy)
That said, we have no "new document" in our context where we really need a preset. We have only fixed types of documents and I have doubts that it would make sense to view one C++ file with "this" and another C++ file with "that" settings.
So, how about to remove that page?
At least should there "Display functions parameters" be removed. It's only supported by three parser: cpp, php, ruby
Later I want remove/replace the fixed options m_macro, m_struct, m_func with some more flexible and fitting solution. I think I have made a FIXME note about this.
Greetings, Lothar
Comment Actions
But now I'm curious which part of my text was so persuasive.
I'm objecting to the removal of the settings page that is not part of this patch.
Comment Actions
Hi,
I was just about to commit this change when I noticed that:
- The "Expand Tree" option is enabled at startup even if tree-mode is not enabled.
- If I have "Expand Tree" disabled, no active symbol, a one tree expanded and I toggle "Show Parameters" it will collapse the tree.
- Toggling off "Expand Tree" does not collapse all trees. It seems it first collapses all and then expands the tree of the current item. Actually I noticed that this patch does not introduce this "feature".
Can you fix at least 1)?
- and 3) might require a bit more changes to get them to work logically and this patch is not introducing the problem.
Regards,
Kåre