It makes no sense to have them not there or to treat them special
- Disable menu options dependent from current setting
sars |
Kate |
It makes no sense to have them not there or to treat them special
Lint Skipped |
Unit Tests Skipped |
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.
I think it is ok to add these to the popup.
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
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
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
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
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. |
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 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
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.
Hi,
I was just about to commit this change when I noticed that:
Can you fix at least 1)?
Regards,
Kåre