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

Various fix/enhancement about Profiles #8726

Merged
merged 41 commits into from
Aug 25, 2023
Merged

Conversation

Clem-Fern
Copy link
Contributor

@Clem-Fern Clem-Fern commented Jul 20, 2023

Hi @Eugeny, hope you're doing great !

I'm currently working on a small list of issues which are related to Profiles and that I want to try to address. I'm opening this draft to keep track on my progress, and I will probably ask for your opinion on the way to implement some feature if you are ok with that.

Resolve #7057 4dedbbc: As recentProfiles already are sort by most recent used profile, I simply add weight on the SelectorOption based on the profile index in the recentProfiles array.

Fix #8709 d57757c: I made a small mistake previously. The weight I added on the Connect to selector option were not used in the onFilterChange function in selectorModal component.

Edit 11/08:
Resolve #3999 695c5ba : allow users to specify settings on profile group for each profile provider

Edit 12/08
Resolve #7265 f369b14 : added a button to reset global default profile settings and to restore inherited settings on a group.

Edit 15/08
Fix #8065 a9c63b5: This commit fixes the issue #8065. Resolve the returned Promise in showProfileSelector when selector modal is dismissed.

@Eugeny
Copy link
Owner

Eugeny commented Jul 21, 2023

LGTM so far 👍

@Clem-Fern
Copy link
Contributor Author

Clem-Fern commented Jul 23, 2023

Hey @Eugeny,

I've done a bit of refactoring on Profile & ProfileGroup system, I tried to move all the interaction (creation, update, deletion) with Profile and ProfileGroup in ProfileService as discuss in #8663. My goal is to make maintenance and future work easier by having only one entrypoint for profile operation. With the methods in the core service, It will allow me to remove direct access to config.store from profilesSettingsTab.component.ts.

I also tried to prepare implementation of #3999, #3880, #7265 and #6650 by reworking ProfileGroup. Should I create a dedicated service for Group ? (ProfileGroupService for example)

Could you, please, take a look when you got the time ? Even if I'm far from done, I would reallly like to have your feedback on this :)
You will probably have a better approach than mine to suggest

.
P.S.: I just found a little glitch with the group selector in EditProfileModal, I will fix that

@Eugeny
Copy link
Owner

Eugeny commented Jul 24, 2023

Wow! I haven't had time to review it in detail yet, but I like your direction.

Small nitpick: I wouldn't keep the collapsed status in the config, it's only relevant for the profileSettingsTab anyway.
What's the reason for writeProfile deleting the fields one by one? Is it so that default values get removed? That should already work automatically via ConfigProxy.

One thing to keep in mind (now that you have 3 config "levels") is what's going to happen if a profile setting is changed to non-default in the group config, but then is changed back to default in one of its profiles. Right now ConfigProxy will remove the value completely since it matches the defaults. You'll need to make sure that getProfileDefaults includes group config as the profile's "defaults".

@Clem-Fern
Copy link
Contributor Author

Clem-Fern commented Jul 24, 2023

Wow! I haven't had time to review it in detail yet, but I like your direction.

Glad to hear that ! ;)

Small nitpick: I wouldn't keep the collapsed status in the config, it's only relevant for the profileSettingsTab anyway.

Oh yeah, seems obvious now you say it.

One thing to keep in mind (now that you have 3 config "levels") is what's going to happen if a profile setting is changed to non-default in the group config, but then is changed back to default in one of its profiles. Right now ConfigProxy will remove the value completely since it matches the defaults. You'll need to make sure that getProfileDefaults includes group config as the profile's "defaults".

I haven't really looked into it yet but it was my next step planned.

@Clem-Fern
Copy link
Contributor Author

Clem-Fern commented Jul 24, 2023

What's the reason for writeProfile deleting the fields one by one? Is it so that default values get removed? That should already work automatically via ConfigProxy.

When a profile is updated, the group attribute is set to undefinied if no group has been selected. Because of that, Object.assign do not seem to override the group attribute and leave the old value. It made impossible to reassign a profile in Ungrouped. But, now at head rested, I think this behavior only happens with the group attribute. It would probably be easier to change a bit the EditProfileModal to leave an empty string in the group attribute when no group has been selected.

@Clem-Fern
Copy link
Contributor Author

Hi @Eugeny, I hope you are doing great :)

Resolve #3999 695c5ba : With this commit, I got a first usable version allowing users to specify settings on profile group for each profile provider. I will use this branch as my daily to dig up any bug.

@Clem-Fern
Copy link
Contributor Author

Clem-Fern commented Aug 14, 2023

No problem, there's absolutely no rush. I got two more issues I would like to address so I will stop adding new commit soon anyway. It's a good first step for me I think and I really don't want to make this PR a chore to review for you :)

@Clem-Fern
Copy link
Contributor Author

Clem-Fern commented Aug 14, 2023

ef6b8a4 : I made a bit of refactoring to be aligned with my previous PR #8416. I created an interface named ConnectableProfile which inherit the Profile interface attributes. I was thinking that it could be useful to be able to declare properties on connectable profile only (in anticipation of ef6b8a4). To go with this change, I also create an abstract class ConnectableProviderProfile and moved quickConnect and intoQuickConnectString methods in it as the quick connect capability only concerns connectable profiles. I updated serial/ssh/telnet session, tab component and provider class to concur with this refactoring.

ef6b8a4 : I added a new property in ConnectableProfile named clearServiceMessagesOnConnect. My goal was to replace the clearServiceMessagesOnConnect specific ssh option and provide a per connectable provider type option. The clearServiceMessagesOnConnect attribute is now read in the initializeSession method and the terminal cleared according to the attribute value. As this feature did not exists before for the telnet and serial session, the default value was setted to false.

5eeaef9 : I removed the old clearServiceMessagesOnConnect option and added a new config migration to suit the previous commits. The old clearServiceMessagesOnConnect property is transferred into the ssh provider profile defaults.

Edit 18/08:
7687972: Refactoring ef6b8a4. Create two distinct class -> ConnectableProfileProvider extends ProfileProvider with generic type which extends ConnectableProfile and
QuickConnectProviderProfile extends ConnectableProfileProvider with quick connect methods. (for example: useful with serial as SerialProvider gives ConnectableProfile but does not have quick connect capability.

@Clem-Fern
Copy link
Contributor Author

Clem-Fern commented Aug 15, 2023

Fix #8065 a9c63b5: This commit fixes the issue #8065. While trying to understand why some button became unresponsive when Profiles & Connections selector is closed without selecting anything, I found that dismissing the selector modal was not handle in showProfileSelector. It causes the returned promise to not being resolved or rejected.

Even if unhandled rejection on modal dismissed does not seem to have impact anywhere else than for #8065, I also tried to add a catch call to handle Promise rejection. It avoids raising Uncaught (in promise) exception and made the log a bit cleaner.

No more addition planned on this PR. From now, I will only add fix on what I've done if needed :)

@Clem-Fern Clem-Fern changed the title [WIP] Various fix/enhancement about Profiles Various fix/enhancement about Profiles Aug 15, 2023
@Clem-Fern Clem-Fern marked this pull request as ready for review August 15, 2023 18:26
tabby-core/src/services/commands.service.ts Outdated Show resolved Hide resolved
tabby-core/src/services/profiles.service.ts Outdated Show resolved Hide resolved
tabby-core/src/services/profiles.service.ts Outdated Show resolved Hide resolved
tabby-core/src/services/profiles.service.ts Outdated Show resolved Hide resolved
tabby-core/src/services/profiles.service.ts Outdated Show resolved Hide resolved
tabby-core/src/services/profiles.service.ts Outdated Show resolved Hide resolved
tabby-core/src/services/profiles.service.ts Outdated Show resolved Hide resolved
tabby-core/src/api/profileProvider.ts Show resolved Hide resolved
@Eugeny
Copy link
Owner

Eugeny commented Aug 24, 2023

I've gone through it and left some nitpicks here and there but all in all the PR looks great already 👍

@Eugeny
Copy link
Owner

Eugeny commented Aug 25, 2023

Phew what a huge merge 😅

@Issues-translate-bot
Copy link
Collaborator

The translator bot has detected that this issue body's language is not English, and has translated it automatically.


Phew what a huge merge 😅

@Eugeny Eugeny merged commit 4684b0d into Eugeny:master Aug 25, 2023
10 checks passed
@Clem-Fern
Copy link
Contributor Author

Hey, I'm sorry, I didn't have the time to respond and fix everything in one time...
Thank you for taking the time to review this and for finishing cleaning my mess ahah

Refactoring profiles service was the first step to achieve before being able to work on profiles/groups tree hierarchy. I would like to work on this next (if you agree ofc).

@Eugeny
Copy link
Owner

Eugeny commented Aug 25, 2023

Thanks for undertaking the work haha

I'm fine with nested groups as long as it's not negatively affecting the "normal" case that 90% of users will have (no nested groups), e.g. so that the profile selector doesn't end up being some unwieldy MobaXTerm-esque control

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