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

[WIP] Refactor #937

Closed
wants to merge 25 commits into from
Closed

[WIP] Refactor #937

wants to merge 25 commits into from

Conversation

auxves
Copy link
Contributor

@auxves auxves commented Jul 2, 2019

Short description of what this resolves:

This PR separates different functionality into their own services to refactor the code.

Changes proposed in this pull request:

  • Move sync.ts upload and download functionality and all gist functionality to gist.service.ts
  • Move all settings-related functionality to settings.service.ts
  • Move all output related functionality to logger.service.ts
  • Move all auto-upload related functionality to autoUpload.service.ts
  • Move localization-related functionality to localization.service.ts
  • Move initialization code to init.service.ts
  • (probably more soon)

What definitely still works

  • Auto Upload
  • Settings GUI
  • OAuth Login
  • Gist Selection
  • Upload
  • Download
  • Public Gist
  • Ask Gist Name
  • Force Upload
  • Force Download

Fixes: #935

How Has This Been Tested?

I have tested the functionality mentioned above, but it needs a lot more testing.

Checklist:

WARNING: This is a VERY EXPERIMENTAL PR and has LOTS of changes (for developers. end-users shouldn't be affected).

@shanalikhan
Copy link
Owner

As per discuss with the author of #822. Following are the main objectives for refactoring.

  1. "I think it’d be better if the interface is named ISyncService and only has the upload and download functions. That way, it’ll be a lot easier when implementing Git to just implement the download / upload functions and use factory creation to create the sync service"
  2. There will be setting same thing named as exportType that should be set by default as "GithubGist" that will be future changed to either Git, File.
  3. Each export type will have their own settings we need to change the current GithubGist to new setting, hence the new settings will look like this.

Change the Global Settings like

{
    "ignoreUploadFiles": [
        "state.vscdb",
        "state.vscdb.backup",
        "syncLocalSettings.json",
        ".DS_Store",
        "sync.lock",
        "projects.json",
        "projects_cache_vscode.json",
        "projects_cache_git.json",
        "projects_cache_svn.json",
        "gpm_projects.json",
        "gpm-recentItems.json",
        "state.*"
    ],
    "ignoreUploadFolders": [
        "workspaceStorage"
    ],
    "ignoreExtensions": [],
    "exportType" :"GitHubGist",
    "GitHubGist":{
        "gistDescription": "Visual Studio Code Settings Sync Gist",
        "token": "998a2ee5389f61d63d729ed62a261b1afaad8279",
        "downloadPublicGist": false,
        "openTokenLink": true,
        "lastUpload": "2019-06-24T16:18:55.696Z",
        "lastDownload": "2019-06-24T16:19:50.647Z",
        "githubEnterpriseUrl": null,
        "askGistName": false,
        "hostName": null,
        "autoUploadDelay": 20
    },
    "version": 340,
    "supportedFileExtensions": [
        "json",
        "code-snippets",
        "json2"
    ],
    "disableUpdateMessage": false,
    "customFiles": {},
    "universalKeybindings": false
}

@auxves
Copy link
Contributor Author

auxves commented Jul 2, 2019

@shanalikhan

only has the upload and download functions. That way, it’ll be a lot easier when implementing Git to just implement the download/upload functions.

I have also added IsConfigured() to the interface to make it easier to check if that service is configured properly.


I was also thinking of removing some items from the Advanced Options menu because you can change them in the Settings UI and it would unclutter the settings service a lot.

"cmd.otherOptions.openSettingsPage",
"cmd.otherOptions.editLocalSetting",
"cmd.otherOptions.shareSetting",
"cmd.otherOptions.downloadSetting", **Can be toggled from Settings**
"cmd.otherOptions.toggleForceDownload", **Can be toggled from Settings**
"cmd.otherOptions.toggleForceUpload", **Can be toggled from Settings**
"cmd.otherOptions.toggleAutoUpload", **Can be toggled from Settings**
"cmd.otherOptions.toggleAutoDownload", **Can be toggled from Settings**
"cmd.otherOptions.toggleSummaryPage", **Can be toggled from Settings**
"cmd.otherOptions.customizedSync",
"cmd.otherOptions.downloadCustomFile",
"cmd.otherOptions.joinCommunity",
"cmd.otherOptions.openIssue",
"cmd.otherOptions.releaseNotes"

What do you think?

@auxves auxves mentioned this pull request Jul 2, 2019
3 tasks
auxves added 2 commits July 2, 2019 14:22
webview.service.ts
----
Read version from package.json
Update corresponding settings to match new sync service configs

environment.ts
----
Read version from package.json

init.service.ts
settings.service.ts
----
Fix issue with IsConfigured()
@shanalikhan
Copy link
Owner

Yes IsConfigured() is fine.

I was also thinking of removing some items

Yes as we have UI now, most of the things can be managed from there. Its safe to remove them.

auxves added 5 commits July 2, 2019 14:42
gist.service.ts
----
Fix issue with auto upload

settings.service.ts
----
Remove settings from advanced options menu
@auxves
Copy link
Contributor Author

auxves commented Jul 8, 2019

@shanalikhan Since we added the buttons to Donate or Write a Review in the landing page, maybe we should remove DonateMessage() from the codebase. Judging from the community's response to #713, it's clear that Settings Sync already has lots of distracting and somewhat annoying pop-ups that could lead to worse user experience. What do you think?

References:

  • if (state.context.globalState.get("syncCounter")) {
    const counter = state.context.globalState.get("syncCounter");
    let count: number = parseInt(counter + "", 10);
    if (count % 450 === 0) {
    this.DonateMessage();
    }
    count = count + 1;
    state.context.globalState.update("syncCounter", count);
    } else {
    state.context.globalState.update("syncCounter", 1);
    }
  • public async DonateMessage(): Promise<void> {
    const donateNow = localize("common.action.donate");
    const writeReview = localize("common.action.writeReview");
    const res = await vscode.window.showInformationMessage(
    localize("common.info.donate"),
    donateNow,
    writeReview
    );
    if (res === donateNow) {
    vscode.commands.executeCommand(
    "vscode.open",
    vscode.Uri.parse(
    "https://www.paypal.com/cgi-bin/webscr?cmd=_donations&business=4W3EWHHBSYMM8&lc=IE&item_name=Code%20Settings%20Sync&item_number=visual%20studio%20code%20settings%20sync&currency_code=USD&bn=PP-DonationsBF:btn_donate_SM.gif:NonHosted"
    )
    );
    } else if (res === writeReview) {
    vscode.commands.executeCommand(
    "vscode.open",
    vscode.Uri.parse(
    "https://marketplace.visualstudio.com/items?itemName=Shan.code-settings-sync#review-details"
    )
    );
    }
    }

@shanalikhan
Copy link
Owner

Lets keep this now, we will look into it later how we can improve it.

auxves added 5 commits July 8, 2019 14:33
util.ts
----
Merge

gist.service.ts
----
Merge
Implement Reset method to reset state

sync.model.ts
----
Add Reset method to interface to allow sync services to reset custom properties

settings.service.ts
----
Call Reset method when resetting
package.nls.*.json
----
Remove unused localizations

gist-config.model.ts
----
Remove unused setting
Rename githubEnterpriseUrl to githubEndpoint
@shanalikhan shanalikhan changed the base branch from v3.4.0 to master July 16, 2019 06:30
@shanalikhan
Copy link
Owner

I suggest you to close this PR.
and rebase your commits into the latest master branch and then fix the conflicts. In this way we will be sure that no feature/code is missing from whole code refactoring.

@auxves auxves closed this Jul 22, 2019
@shanalikhan
Copy link
Owner

@arnohovhannisyan

Any update on this ?

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.

2 participants