Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workspace-grid@hernejj: Fix workspace-grid for Cinnamon 5.4 #4405

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

rcalixte
Copy link
Member

Tested the code on a local workstation and confirmed that it works for Cinnamon 5.4.7.

cc: @hernejj

@brownsr brownsr merged commit 556d5f5 into linuxmint:master Aug 2, 2022
@ghost
Copy link

ghost commented Aug 2, 2022

Is this applet now still compatible with any Cinnamon older than 5.2? Or any Cinnamon at all...?
For what it's worth in Mint 19.2 an attempt at enabling this applet results in the following error:

error t=2022-08-02T13:45:15Z [undefined]: File not found: /home/dragos/.local/share/cinnamon/applets/workspace-grid@hernejj/applet.js

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

It was working fine for me on Cinnamon 5.2. I had issues with it on Cinnamon 3.6.7.

@Drugwash2 The path in that error indicates it might be an issue with the filesystem. The path is 5.4 or 5.2 (but if we should lower 5.2 to be compatible with Mint 19.2's version of Cinnamon, that can be done).

There are likely to be other PRs I'll open as I have a few features I'm working on so please shout if something needs tuning.

@ghost
Copy link

ghost commented Aug 2, 2022

There are no issues with the file system. The original applet works fine in Mint 19.2/Cinnamon 4.2 since it does have the main applet.js in its root folder. Could be a change in Cinnamon 5.x that allows a different (sub)folder structure, but as it is now the backward compatibility is broken.

I also have a customized version of this applet where I chose a different approach for overall compatibility. That version works fine in both Mint 19.2 (Cinnamon 4.2) and Mint 21 (Cinnamon 4.5). Please see here.
I will change its name to workspace-grid-icons in order to avoid overlapping with the original.

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

@Drugwash2 What I meant was with the new directory structure. For the sake of backward compatibility, if the directory needs to be renamed from 5.2 to 4.2 or 4.5, that change can and probably should be made.

@ghost
Copy link

ghost commented Aug 2, 2022

For what it's worth I think this change is a very bad thing, but not gonna elaborate on this now - would be useless anyway.
The minimum version should be first tested on a live system, but either way the older systems (Cinnamon versions) wouldn't know about the new folder structure so they would still fail.

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

but either way the older systems (Cinnamon versions) wouldn't know about the new folder structure so they would still fail.

I don't know that this latter statement is true? Take a look at https://github.com/linuxmint/cinnamon-spices-applets/tree/master/sound150@claudiux/files/sound150@claudiux. This is why I was asking about your local configuration as this is a convention that has been in place for quite some time.

Correct me if I'm wrong but I believe the only action we need here is to determine the correct minimum version. (When I opened the PR, I was under the impression this applet was broken for 4.8.)

@ghost
Copy link

ghost commented Aug 2, 2022

Ah, you're right, the structure seems to have been in place for some time. My bad.
So it should only be about the minimum compatible version.
Unfortunately I don't know any quick and certain method to check compatibility against multiple Cinnamon versions in order to determine the minimum compatibility. All I have at hand is this 19.2/4.2.4, a virtual 19.3/4.4 and a 21beta/5.4.8.

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

I can set aside some time to play with it as I would like to ensure backward compatibility remains in tact for supported versions. I'm also willing to contribute fixes for the supported versions as well when that testing occurs. This might take a few days on my end.

One side note, I noticed your fork and it seems like a worthy upgrade. Would you like to rebase your branch and then open a pull request? It seems like it would fit into a 4.2 directory while also being applied to what we have in 5.2 and 5.4.

@ghost
Copy link

ghost commented Aug 2, 2022

If you could test with various older Cinnamon versions that would be great. I don't have any kind of free space left even for virtual installations.

Thing is, Mint 19 will go out of support in a few months but that doesn't mean everybody will instantly give it up - if ever. I for one will not, partly because the hardware is old and doesn't play well with newer Mint, partly because certain changes in versions from 19.3 up have crippled the desktop experience as I need it.

Also we never know how many people may still use versions even older than 19.x, for whatever reason. As such it would be a courtesy to allow people using older systems the chance to install and use this applet - and any others if possible. I've been a Windows 98 user for a very long time and I know how important it is to care about all people, not only those running on the bleeding edge.

As for my fork, first thing is I can't work with this platform for the life of me. No idea what tools are being used and how, no idea how to rebase, open pull requests and all that; I've been working by myself offline since forever, all this is new to me and I'm not young anymore to quickly adapt to changing environments.

So, if you want you can simply copy the code from my repository and implement it in the official Mint branch - or however it's being called. I'm not sure if it will be too well received considering there is no option to disable icons or numeric badges should one not want/need those. But if you think you can deal with that and make it optional then please go ahead and play with the code.

@in-in
Copy link

in-in commented Aug 2, 2022

This applet stopped working on my system after the last update. And I see no way to downgrade to the previous version.
22214202857
22214202927
22214203032

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

A lot to digest there. 😅

I'll be honest and say that I am somewhat conflicted about what the best path for what will be unsupported should be. (A hard line should be drawn when things are no longer supported otherwise the designation carries no significance.) That being said, I also don't think we should deliberately break older versions which I think we can avoid here. Best efforts seems like the happy middle-ground. As I noted earlier, this applet did not work for me on Cinnamon 3.6.7 (I haven't dug into it yet) and I thought there were issues with 4.8 (more testing to determine if this is true or not). Mint 19.3 features Cinnamon 4.4 so we should strive to make sure it works with that. (And once it's working, it hopefully stays working.)

Regarding the code integration, I'll spend some time to get it done, adding options for the new features along the way.

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

@in-in You should be able to workaround this by going to your applets directory and renaming 5.2 to 5.0. I'll open a PR for this now. I think I'll go with 4.2 as that is what shipped with Mint 19.2.

@ghost
Copy link

ghost commented Aug 2, 2022

@rcalixte For larger compatibility range you could take a look at the solution I adopted in my fork; see WorkspaceController.js.
Basically it's testing for a method (global.workspace_manager) introduced in Cinnamon 5.2 or 5.4 and based on that it uses either global.screen.override_workspace_layout(Meta.ScreenCorner.TOPLEFT or global.workspace_manager.override_workspace_layout(Meta.DisplayCorner.TOPLEFT without the need to duplicate the file. This way it works for me in Cinnamon 4.2.4 and in 5.4.8 so it should hopefully work correctly in all other versions in between.

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

Correct. I noticed that. When I have time to refactor, I'll take that into account. Separating things by version enables future development not to conflict with what will be stable for end users. If we take the path of adding conditionals based on version, that will likely grow to be untenable over time.

I figure I should have some time starting on Friday and hopefully this weekend so we can refactor this and roll out the update soon. I'm excited for the changes! (There are other changes I would like to make for 5.4 that I believe rely on newer features and it would be easier to develop/test without having to worry about impact to versions I'm not running.)

@ghost
Copy link

ghost commented Aug 2, 2022

Separating things by version enables future development not to conflict with what will be stable for end users.

That may be true but it also negates any local unofficial changes/fixes/improvements that one could make to their system. It's the same thing that happened in Windows9x where the system was heavily updated through various means (KernelEx, manual library updates etc) which brought newer APIs but the obstinate version check in applications still deemed them incompatible although they would run fine. Thankfully there was the compatibility switcher in KernelEx that could fake any higher OS version - but there is no such thing in Linux.

If we take the path of adding conditionals based on version, that will likely grow to be untenable over time.

Actually conditionals are based on presence or absence of classes/methods themselves, not on version numbers. That ensures correct functionality regardless of Cinnamon or other libraries' version numbers, and also allows unofficial changes/additions to be used.

For example I have locally added a few methods to settings: getRange(), rangeCheck() and reset(). Suppose Mint devs would implement these in say Cinnamon 6.0. All x-lets using those methods would instinctively up the requirements to Cinnamon 6.0 because that's where they know it started. But if I already have those in Cinnamon 4.2 or manually implement them at a later time I'd already be cut out from installing and using those x-lets because they would only test for Cinnamon version, not for the actual presence of those methods. I hope you understand what I'm saying.

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

I understand what you're saying. We're essentially discussing the best strategy for supporting older versions of Cinnamon. My take is that since 4.2 will be EOL in April 2023, it makes sense to branch off there. If there are breaking ABI changes introduced in 6.0 or 5.6, testing for 4.2 or 4.4 becomes complicated.

Here is what I am envisioning:
4.2, where we roll in the features in your branch and keep compatibility (by minimizing future changes to focus on stability)
5.4, where we roll in the new features but also extend functionality (similar to other applets where development is broken out for future versions)

If 5.8 introduces backwards incompatibility, that work should be focused in a 5.8 path instead of the singular path that could cause issues for 5.4 and/or 4.2+. I should also add that in my case, I only have 5.4 available for testing purposes but there's no reason we can't backport to other versions.

Do you agree with this path?

@ghost
Copy link

ghost commented Aug 2, 2022

Well, you're the one working with the official line so it's all up to you. But basically yes, your reasoning is sound and acceptable.

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

Ha. I might be doing the work but it impacts all of us! I think we all want to have the best experience, it's just a matter of how we get there. I'll ping you in the PR when it's opened. Do you think you'll have time to test? I want to make sure everything works as intended before we merge.

@ghost
Copy link

ghost commented Aug 2, 2022

Hehe, I hope you realize what's good/best for you may not be good/best for me, and the other way around - generally speaking. 😉

I can test here in 19.2 if it doesn't take too long until it's published. Will have to prepare another large cup of coffee though - soon will already be tomorrow here. 😁

@rcalixte
Copy link
Member Author

rcalixte commented Aug 2, 2022

Haha, cheers! I'm hoping to have it done before the end of the weekend at the latest, ideally sooner.

@ghost
Copy link

ghost commented Aug 2, 2022

I might be out of coffee by then. Oh well... 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants