search

oae / gnome-shell-pano

Next-gen Clipboard Manager for Gnome Shell
https://extensions.gnome.org/extension/5278/pano/
GNU General Public License v2.0
978 stars 52 forks source link

update to gnome 46 #264

Closed underdoeg closed 3 months ago

underdoeg commented 3 months ago

Pull Request Template

Description

Simple update for gnome 46. Seems to work but was not thoroughly tested.

Fixes #259

Type of change

Checklist

Totto16 commented 3 months ago

I would suggest, that we keep backwards compatibility with gnome 45, since we did that before, and it's best practice, it is detectable, if add_actor or add_child ( in a helper function) is present and so we keep compatibility, The only reason, that we didn't do that from gnome 44 -> 45, is that there were many significant changes there.

underdoeg commented 3 months ago

AFAIK in gnome 45 add_actor was already deprecated and add_child should work as well.

lkraav commented 3 months ago

Yes, other work in the wild confirms 45 and 46 both being fine with this upgrade https://github.com/fflewddur/tophat/pull/122

Totto16 commented 3 months ago

Another note from me, since it's not mentioned, EGO Reviewers said, that we need to use fewer third party libraries (see e.g. https://github.com/oae/gnome-shell-pano/pull/222#issuecomment-1812213024), so this PR is ready to merged imo @oae . But before submitting a new version for gnome 46 we need to address this issue, did you already think of any solution for that @oae ?

It's unrelated to this PR, but i'll say it anywhere, do we now what the exact reasoning behind that statement from EGO is, and what are we supposed to do in their view, since we definitely need these third party dependencies (e.g. highlightjs)?

oae commented 3 months ago

Another note from me, since it's not mentioned, EGO Reviewers said, that we need to use fewer third party libraries (see e.g. #222 (comment)), so this PR is ready to merged imo @oae . But before submitting a new version for gnome 46 we need to address this issue, did you already think of any solution for that @oae ?

It's unrelated to this PR, but i'll say it anywhere, do we now what the exact reasoning behind that statement from EGO is, and what are we supposed to do in their view, since we definitely need these third party dependencies (e.g. highlightjs)?

I don't know the exact reason, but I think third-party libraries in Pano make the reviews very difficult for them. So, they don't want me to include them anymore. I didn't have time to think of a solution. If we can load these libraries dynamically somehow, we can ask the user to accept them during the first start and download them from Github. But at this point, this extension becomes a glorified installer. Maybe it is better to not update on EGO anymore.

oae commented 3 months ago

@underdoeg thank you for the pr <3

Totto16 commented 3 months ago

Another note from me, since it's not mentioned, EGO Reviewers said, that we need to use fewer third party libraries (see e.g. #222 (comment)), so this PR is ready to merged imo @oae . But before submitting a new version for gnome 46 we need to address this issue, did you already think of any solution for that @oae ? It's unrelated to this PR, but i'll say it anywhere, do we now what the exact reasoning behind that statement from EGO is, and what are we supposed to do in their view, since we definitely need these third party dependencies (e.g. highlightjs)?

I don't know the exact reason, but I think third-party libraries in Pano make the reviews very difficult for them. So, they don't want me to include them anymore. I didn't have time to think of a solution. If we can load these libraries dynamically somehow, we can ask the user to accept them during the first start and download them from Github. But at this point, this extension becomes a glorified installer. Maybe it is better to not update on EGO anymore.

Or we create a deb / rpm / whatever (depending on the distro) for the needed files, and you need to install those, but thats equally bad...

Regarding https://github.com/oae/gnome-shell-pano/pull/222#issuecomment-1812213024

I would ask you @jrahmatzadeh (the author of that comment )

I read the guidelines

And I found Use of external scripts and binaries is strongly discouraged. In cases where this is unavoidable for the extension to serve it's purpose [...]

Which to me sounds like we are in that area, since we need those external scripts /dependencies for pano to work...

There is this section: Extensions may install modules from well-known services such as pip, npm or yarn but MUST require explicit user action. For example, the extension preferences may include a page which describes the modules to be installed with a button.

Is there an example, how one can do that, without forcing the user to install node, npm and the dependencies we need, and than we need to use some version of these dependencies, we don't know, it could be, that we are compatible with the version, the user installed, but it also could not be. It also could be, that the user needs another version for his local development or some similar reasons. So its 100% better imo to include prepackaged dependecies, that work with the extension than doing it this way. We speak about 37 external files atm, so it's not a few 100 / 1000 external scripts, we could bake those dependencies in the extension or maybe reduce it by 1 or 3 files, but the highlight_js dependency is essential to the functionality of this extension...

I am an user, that can do the steps like installing global fixed version packages from npm, but not every user can do that and shouldn't have to. On EGO it says 110617 Downloads so this isn't an unpopular extension, so there has to be a way to fix this and resolve that issue to republish it on EGO. Also Ubuntu 24.04 is an LTS and ships Gnome 46, so there are many users, that might use this extension on gnome 46.

@jrahmatzadeh You said, you discussed it in the matrix channel, is there a chance we can find a solution for this together, I get your point of using dependencies / external scripts, but there has to be a solution? (Sorry if it came over harsh, but I used gpaste and other similar extensions than this one, and this is by far the best one ❤️ , so I want it to be available on EGO and accessible to users)

jrahmatzadeh commented 3 months ago

The extension description already mentions the libgda dependency. You can add other dependencies in the description too.

I believe loading many third party libraries in GNOME Shell process isn't good either.

Btw, have you considered moving this project to a gjs app rather than an extension? That way, you will be able to do whatever you wanna do in your app outside the shell process and the GNOME Shell extension can only implement the indicator part. It can be available on other desktops and even other OSs. For example, kando is a project based on Fly-Pie extension.

Totto16 commented 3 months ago

The extension description already mentions the libgda dependency. You can add other dependencies in the description too.

I believe loading many third party libraries in GNOME Shell process isn't good either.

Btw, have you considered moving this project to a gjs app rather than an extension? That way, you will be able to do whatever you wanna do in your app outside the shell process and the GNOME Shell extension can only implement the indicator part. It can be available on other desktops and even other OSs. For example, kando is a project based on Fly-Pie extension.

Long term it is possible to rewrite it, to be an app, but short term it's not easy, we understand, that using too many third party dependencies is not the best option, but we rely on some for the extension to work, we will discuss this and find a solution, that either satisfies the rules of EGO or we distribute it in some other way.

It is certainly possible to make the user install those and explain why those dependencies are needed to work, but that requires some thought and intuitive UI or package, using global npm packages is the worst solution possible.

Just one question, before we will discuss the rest internally and maybe reach out to you at the end. (Btw it's not the best for both of us, to just ping you @jrahmatzadeh all the time, is there way we could communicate better, without always "bothering" you personally?)

The question is, how much is too many? We currently use those files as part of the extension thirdparty folder:

date_fns_formatDistanceToNow.js
date_fns_locale.js
hex_color_converter.js
highlight_js_lib_core.js
highlight_js_lib_languages_bash.js
highlight_js_lib_languages_c.js
highlight_js_lib_languages_cpp.js
highlight_js_lib_languages_csharp.js
highlight_js_lib_languages_dart.js
highlight_js_lib_languages_go.js
highlight_js_lib_languages_groovy.js
highlight_js_lib_languages_haskell.js
highlight_js_lib_languages_java.js
highlight_js_lib_languages_javascript.js
highlight_js_lib_languages_julia.js
highlight_js_lib_languages_kotlin.js
highlight_js_lib_languages_lua.js
highlight_js_lib_languages_markdown.js
highlight_js_lib_languages_perl.js
highlight_js_lib_languages_php.js
highlight_js_lib_languages_python.js
highlight_js_lib_languages_ruby.js
highlight_js_lib_languages_rust.js
highlight_js_lib_languages_scala.js
highlight_js_lib_languages_shell.js
highlight_js_lib_languages_sql.js
highlight_js_lib_languages_swift.js
highlight_js_lib_languages_typescript.js
highlight_js_lib_languages_yaml.js
htmlparser2.js
is_url.js
pretty_bytes.js
prismjs.js
validate_color.js

Some of those are essential for the extension and their name already say, what they are doing. Quite a few files are highlight_js, which we use to highlight and detect code snippets. If we would exclude that library and all its files and add it as optional dependency, that an user could install (since most user who need code highlighting are certainly capable of doing some thing to install that in some way). Would that be still to many? The thing is, that these libraries are widely used and tested enough in the wild, that I myself would say, it poses no risk to use them. There is also no security concern in them and they do not do shady things.

By "just" excluding highlight_js we would be able to keep the core functionality but still offer a functional extension to gnome users on gnome 46, and since Linux keeps growing, and you can't say that 100% of the Linux users are solely "tech nerds", that would be better, than offering an app or a package that they have to install. (just my opinion)

Thank you in advance for your patience and your answers.

jrahmatzadeh commented 3 months ago

I'm almost always on GNOME Extensions Matrix Channel.

And yeah, removing code highlighter can be sufficient to pass the review (read the discussion on Matrix).

oae commented 3 months ago

I am ok with this. The code type in the Pano is not accurate anyway. We can remove it.

Totto16 commented 3 months ago

It needs some overhaul to be really helpful (e.g. displaying the language it is in) than it would be helpful (at least for me) So I would say, we disable it for now, but overhaul it, so that an user can install some things manually, to enable it back again. I would do the work, if you don't want to @oae

oae commented 3 months ago

Thank you <3

Totto16 commented 3 months ago

I'm almost always on GNOME Extensions Matrix Channel.

And yeah, removing code highlighter can be sufficient to pass the review (read the discussion on Matrix).

I've read the matrix channel, and sorry for asking some duplicate questions, but I didn't read it before 😓

TheLastGimbus commented 3 months ago

I see this is complicated, but just to add my few cents: so far I really liked that Pano highlighted my code syntax. It made it all feel really polished, and made quicly looking through my pastes easy ("oh, some colorful text, must be my code")

But if it must go... then fine :(

Totto16 commented 3 months ago

I see this is complicated, but just to add my few cents: so far I really liked that Pano highlighted my code syntax. It made it all feel really polished, and made quicly looking through my pastes easy ("oh, some colorful text, must be my code")

But if it must go... then fine :(

I also like it and find it useful, so we are going to include it again, but in a different way, and opt in by the user (by e.g. installing a package) This is still WIP, but I like the code highlighting too, so I am definitely adding it back, even if it's opt in this time.