atom-community / markdown-preview-plus

Markdown Preview + Community Features
https://atom.io/packages/markdown-preview-plus
Other
370 stars 88 forks source link

Feature which disable integration with other plugins #509

Closed artctv closed 3 years ago

artctv commented 3 years ago

Your plugin is great! But, for example, I use the plugin tool-bar and the extension plugin flex-tool-bar which allows me to easily modify this panel and I use my own set of buttons, icons and their specific sequence. However, your plugin adds its own icon with callback markdown-preview-plus:toogle if it finds the tool-bar plugin installed. It ruins my set of buttons. I already fiddled with your plugin code, turned off this feature and rebuilt it locally, but your plugin was recently updated, which reversed my changes (( I would like to be able to "checkbox" off the integration with other plugins in general, or with each plugin that markdown-preview-plus reacts to, separately! Thanks!

lierdakil commented 3 years ago

Tool-bar support was implemented by @aminya in #499. I don't use tool-bar, so I have no idea what this is about. Hopefully you two can discuss this and come to some sort of agreement.

From my point of view, the most straightforward option is to revert #499 and say "if you want a button, use flex-tool-bar".

aminya commented 3 years ago

It is reasonable for the plugins to provide their UI elements, not the other way around. If you want to disable Markdown button of flex-tool-bar, you should ask them to remove it since it is already implemented in the original library.

We don't have a standard tool-bar library, but I plan to make one. I will standardize common functionalities (e.g those that would be used by IDE packages) in it. However, markdown preview is "not a standard functionality". It is specific only to this package.

lierdakil commented 3 years ago

@aminya, you misunderstand the issue. flex-tool-bar provides user configurable buttons for tool-bar. I believe that messing with user configuration is bad, bad, bad. So we need to do something here.

aminya commented 3 years ago

Adding a button does not mess with user configuration. It just adds another button to "the end".

lierdakil commented 3 years ago

Yes, but if the user configured tool-bar exactly to their liking, unconditionally adding buttons is "messing with user configuration". For the sake of argument, imagine if every package added a button or two to the tool-bar. The tool-bar would become impossible to navigate.

aminya commented 3 years ago

Well, I have a PR coming for tool-bar that will allow geek users to remove the buttons. But that would be a niche situation. IMO, it is not a good approach to remove a feature for everyone because some geek user has written custom code to do it.

It is similar to status-bar. Everyone is free to add buttons. Each package will do it individually (linter, GitHub, Git, etc).

lierdakil commented 3 years ago

This goes against the tenet of Atom hackability, which is the primary selling point IMO. I don't mind reasonable defaults, but there should be a way to change those. Consider keymaps for instance: packages can provide default bindings, but there's a way to disable those and define your own.

aminya commented 3 years ago

@lierdakil I am working on this. There is an old branch here that allows disabling buttons. https://github.com/suda/tool-bar/tree/features/%2349-plugins-manager

I don't think it is the responsibility of markdown-plus to remove the button.

I recommend adding a simple configuration for now. But eventually, all the buttons will be handled in a unified place (tool-bar itself).

lierdakil commented 3 years ago

Status-bar is slightly different because the UI is entirely software-defined, so it's infeasible to make it particularly customizable beyond moving and hiding elements (which, don't get me wrong, would be a nice feature to have). Tool-bar buttons however are more akin to keymaps than the status-bar.

lierdakil commented 3 years ago

@4rtcrt, 'markdown-preview-plus.disableToolBarIntegration' option has been added in v4.8.0

P.S. Whoops, sorry, messed up in v4.8.0, v4.8.1 should be fine