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

Handle both ferdi and ferdium servers for transferring from #473

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

vraravam
Copy link
Contributor

@vraravam vraravam commented Jul 12, 2022

Pre-flight Checklist

  1. Please remember that if you are logging a bug for some service that has stopped working or is working incorrectly, please log the bug here
  2. If you are requesting support for a new service in Ferdium, please log it here
  3. Please remember to read the self-help documentation - in case it helps you unblock yourself for issues related to older versions of recipes that were installed on your machine. (These will get automatically upgraded when you upgrade to the newer versions of Ferdium, but to get new recipes between Ferdium releases, this documentation is quite useful.)
  4. Please ensure you've completed all of the following.

Description of Change

When checking in pre-existing profiles whether Ferdium is being used without a server, we need to handle both Ferdi as well as Ferdium profiles.

Motivation and Context

Fixes comment from: #4 (comment)

Screenshots

Checklist

  • My pull request is properly named
  • The changes respect the code style of the project (npm run prepare-code)
  • npm test passes
  • I tested/previewed my changes locally

Release Notes

@vraravam vraravam requested review from SpecialAro and a team July 12, 2022 17:13
@vraravam vraravam self-assigned this Jul 12, 2022
Copy link
Member

@SpecialAro SpecialAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I'm not exactly sure if it will suffice to solve the issue. Have you test it?

@SpecialAro SpecialAro merged commit 90afe49 into ferdium:develop Jul 12, 2022
@SpecialAro SpecialAro deleted the fix-4 branch July 12, 2022 23:36
@real-or-random
Copy link

Hi, thanks for the quick attempt to fix this. I tested this with v6.0.0-nightly.100 and it does not work.


A quick code search shows that there is actually another code location where the magic string appears:

export const LOCAL_SERVER = 'You are using Ferdium without a server';

This is used a few places (and should also been used in code that has been touched by this PR).

This location looks like a potential suspect:

server => {
if (server === LOCAL_SERVER) {
ipcRenderer.send('startLocalServer');
}

There's a _migrate() function that's called before that code is reached. I suspect the right thing to do is to add code to this function that rewrites the old string to the new string. Or better, migrate all "You are using ... " some better than a magic string that contains the appname, maybe just the empty string.

If I change the string manually in the settings.json, then everything works. So apparently that setting is really the only culprint. So this should be a reliable way for you to test this. Or alternatively, this also suggests a simpler solution: add a line to the migration shell scripts that rewrites the config...

@vraravam
Copy link
Contributor Author

Good catch and analysis @real-or-random - i will try to fix this later tonight

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.

None yet

3 participants