Some improvement and suggestion #65

mkkeck · 2018-10-23T18:54:39Z

Hi,

I like your project, that's why I've forked and made some changes.

some simplyfied code
cleaned, validated and optimized HTML output
cleaned and optimizes CSS-Files

What I miss:

I18N / internationalization
plugins

I've tried to start I18N, but I don't know how to start. I18Next seems to be good, but I don't need to ship translation for MacOs from Windows and vice versa and of corse the same with other things.

Perhabs, do you have a solution or an good starting point?

Kind Regards

If taskbar in Windows not in light color, it's difficult to see the icon.

Signed-off-by: Michael Keck <github@michaelkeck.de>

coveralls · 2018-10-23T18:59:32Z

Pull Request Test Coverage Report for Build 435

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 424:	0.0%
Covered Lines:	458
Relevant Lines:	458

💛 - Coveralls

oliverschwendener · 2018-10-23T19:29:48Z

Hello there, thank you for your effort! It's nice to see that this project has awakened interest in more people! :)

I will leave a few comments on your code changes later and then we can have a discussion here.

oliverschwendener · 2018-10-23T19:30:01Z

.gitignore

@@ -1,6 +1,13 @@
 .DS_Store
+.idea
+.vscode


I want to have the .vscode folder in the git repo because it contains the debug configurations.

src/ts/app-config/default-app-config.ts

mkkeck · 2018-10-23T19:37:08Z

Okay, but I think there was nothing changes, because I am using Jetbrains IDE.
Or are there something changed through bundler/ builder via Yarn?

oliverschwendener · 2018-10-23T19:48:30Z

src/ts/builders/open-url-with-default-browser-command-builder.ts

@@ -1,6 +1,7 @@
 export class OpenUrlWithDefaultBrowserCommandBuilder {
    public static buildWindowsCommand(url: string): string {
-        return `start explorer "${url}"`;
+        // return `start explorer "${url}"`;
+        return `start "${url}"`;


Some people have problems when executing this command (#42). This is why I changed it to

start explorer ${url}

And by the way I would remove comments like this because it has no use.

oliverschwendener · 2018-10-23T19:49:56Z

src/ts/helpers/ueli-helpers.ts

@@ -1,10 +1,11 @@
 import { join } from "path";
-import { homedir } from "os";
+// import { homedir } from "os";
+import { cwd } from "process";


Also useless comment

oliverschwendener · 2018-10-23T19:50:06Z

src/ts/helpers/ueli-helpers.ts


 export class UeliHelpers {
    public static readonly productName = "ueli";
    public static readonly ueliCommandPrefix = "ueli:";
-    public static readonly countFilePath = join(homedir(), "ueli.count.json");
+    public static readonly countFilePath = join(cwd(), "ueli.count.json");


This also should be in the home directory of the user in my opinion.

oliverschwendener · 2018-10-23T19:51:02Z

src/ts/operating-system-settings/windows/windows-system-commands.ts

@@ -13,19 +13,19 @@ export const windowsSystemCommands: OperatingSystemCommand[] = [
        executionArgument: `${WindowsSettingsHelpers.windowsSettingsPrefix}shutdown -r -t 0`,
        icon: restartIcon,
        name: "Restart",
-        tags: ["reboot"],
+        tags: ["reboot", "neu starten"],


I think multi language support should be done in a nicer way.

oliverschwendener · 2018-10-23T19:52:29Z

src/ts/searcher/file-path-searcher.ts

@@ -51,7 +51,7 @@ export class FilePathSearcher implements Searcher {
                executionArgument: filePath,
                icon: lstatSync(filePath).isDirectory()
                    ? this.iconSet.folderIcon
-                    : this.iconSet.fileIcon,
+                    : this.iconSet.fileIcon, /* TODO differ icons */


No 'to dos' in the code please.

oliverschwendener · 2018-10-23T19:53:15Z

src/ts/user-config/default-config.ts

-import { OperatingSystem } from "./../operating-system";
-import { WindowsIconSet } from "./../icon-sets/windows-icon-set";
-import { MacOsIconSet } from "./../icon-sets/mac-os-icon-set";
+import { platform, release } from "os";


Is this an unused import?

mkkeck · 2018-10-23T20:19:52Z

Okay, I see.

The ueli.config and ueli.count are in current working dir, because I am using the App on USB. Perhaps it would be nice to add a folder like portable, usb or any thing else.

And of corse I've forgotten to remove my testing translations and TODO-comments.

The "release" imported from "os" is required for later testing against versions of Microsoft Windows. Some commands and features are not available on e.g. Windows 7.

The command "start url" instead of "start explorer url" was discused in a Windows-Forum and I've read, that "start url" would be more better. Perhaps that discus does not affect the electron app. Perhabs this should be configurable.

I can and would be able to fix your suggestions tomorrow.

I've forgotten to explain, that I am debugging and developping on Windows 7 and Windows 10 (64bit). For some os commands checks against version and architecture would be needed.

oliverschwendener · 2018-10-24T06:51:08Z

If you want to change the location of your config file, you can change it in the app settings (see #48).

And why is start ${url} better? What is the difference? The only difference I experienced is that for some people it didn't work while start explorer ${url} did. I don't want to change it back only because someone in a forum said "it's better".

Regarding Windows 7: Because this is only a fun project I don't want to spend too much time on supporting a rather old OS. For me it's OK to provide basic funtionality for Windows 7 and make sure that ueli does not crash. But I don't want the effort for Windows 7 support to slow down the development process for ueli ;)

mkkeck · 2018-10-24T09:42:36Z

If you want to change the location of your config file, you can change it in the app settings (see #48).

Okay, I see.

And why is start ${url} better? What is the difference? [...] I don't want to change it back only because someone in a forum said "it's better".

I've not said, it should be switched back. I have some cmd-batches here and read about this in batch programming. For me, it's okay to leave it as it is.

Regarding Windows 7: Because this is only a fun project I don't want to spend too much time on supporting a rather old OS.

For me it's fun too, and of corse I like Ueli :)
That's why I've made some things here.
The suggested check against version of OS should disable things, which are not supported on older OS versions, I don't mean to support them.

Here's a screenshot of an example command on Windows 7 wich results in error:

oliverschwendener · 2018-10-25T06:04:44Z

Oh what I frogot to say: PR's will only be merged into my dev branch.

oliverschwendener · 2018-10-25T06:08:21Z

package.json

    "@types/electron-is-dev": "^0.3.0",
    "@types/electron-store": "^1.3.0",
    "@types/jest": "^23.3.1",
    "@types/mathjs": "^4.4.1",
+    "@types/mocha": "^5.2.5",
+    "@types/node": "^10.12.0",
+    "chai": "^4.2.0",


Where is chai used?

oliverschwendener · 2018-10-25T06:08:45Z

package.json

    "husky": "^0.15.0-rc.8",
    "jest": "^23.5.0",
+    "mocha": "^5.2.0",


Why do we need mocha, when we have jest?

oliverschwendener · 2018-10-25T06:09:01Z

package.json

    "electron-is-dev": "^0.3.0",
    "electron-store": "^2.0.0",
    "electron-updater": "^3.1.2",
    "fuse.js": "^3.2.1",
    "mathjs": "^5.1.1",
+    "minimist": "^1.2.0",


What is this for?

I think this has to do with language and i18n testings

If its not used in the code, then this should be removed

mkkeck · 2018-10-26T12:40:39Z

Hello again,

I've checked out the development branch. The app is opened in full screen mode. How can I resolve this? Or do I have a misstake in my "dev"-branch?

Many thanks.

oliverschwendener · 2018-10-26T13:36:56Z

Can you show me your config?

mkkeck · 2018-10-26T13:44:39Z

Can you show me your config?

Of course, its the default created:

{
  "allowMouseInteraction": false,
  "alwaysShowOnPrimaryDisplay": false,
  "applicationFileExtensions": [
    ".lnk",
    ".appref-ms",
    ".url",
    ".exe"
  ],
  "applicationFolders": [
    "C:\\ProgramData\\Microsoft\\Windows\\Start Menu\\Programs",
    "C:\\Users\\mkeck\\AppData\\Roaming\\Microsoft\\Windows\\Start Menu",
    "C:\\Users\\mkeck\\Desktop"
  ],
  "autoStartApp": true,
  "colorTheme": "dark",
  "customCommands": [],
  "fallbackWebSearches": [],
  "features": {
    "calculator": true,
    "commandLine": true,
    "customCommands": true,
    "email": true,
    "environmentVariables": false,
    "fileBrowser": false,
    "fileSearch": true,
    "operatingSystemCommands": false,
    "operatingSystemSettings": false,
    "programs": true,
    "shortcuts": true,
    "ueliCommands": true,
    "webSearch": true,
    "webUrl": true
  },
  "fileSearchBlackList": [
    "vendor",
    "node_modules",
    "jspm_packages",
    "bower_components"
  ],
  "fileSearchOptions": [
    {
      "folderPath": "C:\\Users\\mkeck",
      "recursive": false
    }
  ],
  "hotKey": "alt+space",
  "iconSet": {
  // [...]
  },
  "language": "auto",
  "logExecution": true,
  "maxSearchResultCount": 8,
  "osLanguage": "de_DE",
  "rescanInterval": 60,
  "searchEngineLimit": 16,
  "searchEngineThreshold": 0.4,
  "searchResultDescriptionFontSize": 14,
  "searchResultHeight": 60,
  "searchResultNameFontSize": 20,
  "shortcuts": [],
  "showSearchResultNumber": true,
  "showTrayIcon": true,
  "smoothScrolling": false,
  "userInputFontSize": 36,
  "userInputHeight": 80,
  "userStylesheet": "",
  "webSearches": [
    // [...]
  ],
  "windowMaxHeight": 560,
  "windowWidth": 860
}

oliverschwendener · 2018-10-26T13:52:01Z

Hmm no idea.. does this only appear when you are on dev?

mkkeck · 2018-10-26T13:59:29Z

Hmm no idea.. does this only appear when you are on dev?

No ;(. Both in Master and Dev.

I think it has something todo with updated electron / node / yarn?

mkkeck · 2018-10-26T14:39:45Z

Hello again,

in function createMainWindow

    mainWindow = new BrowserWindow({
        // [...]
        resizable: true,
        // [...]
    });

solves the problem. But I think it is not correct way?

oliverschwendener · 2018-10-26T14:43:39Z

For me it works just fine. Try this:

    mainWindow = new BrowserWindow({
        autoHideMenuBar: true,
        backgroundColor: "#00000000",
        center: true,
        frame: false,
        height: WindowHelpers.calculateMaxWindowHeight(config.userInputHeight, config.maxSearchResultCount, config.searchResultHeight),
        resizable: true,
        show: false,
        skipTaskbar: true,
        width: config.windowWidth,
+      fullscreen: false
    });

mkkeck · 2018-10-26T15:01:11Z

Yes but only with resizable = true instead of resizable = false

oliverschwendener · 2018-10-26T15:26:03Z

I cloned your repo and everything worked fine (master and dev). What happens if you revert the versions in package.json and yarn.lock to the same as my dev?

mkkeck · 2018-10-26T16:18:12Z

What happens if you revert the versions in package.json and yarn.lock to the same as my dev?
Problem resolved with older Electron.

I've copied version infos:

Works like expected:

ueli: 6.1.2
Electron: 2.0.11
Chrome: 61.0.3163.100
Node: 8.9.3
V8: 6.1.534.41
Index size: 153

Works only with my fix:

ueli: 6.1.2
Electron: 2.0.12
Chrome: 61.0.3163.100
Node: 8.9.3
V8: 6.1.534.41
Index size: 153

oliverschwendener · 2018-10-26T16:26:38Z

electron/electron#14648

mkkeck · 2018-10-26T17:33:07Z

Does this resolve my problem? Or is there a problem with my suggested fix?
Really don't know what you try to say with this link.

oliverschwendener · 2018-10-27T09:11:03Z

In the release notes of electron 2.0.12 is a link to this PR (electron/electron#14648). This confirms that there has been some type of change in electron's code that is related to the resizable API.

However, your fix allows the user to resize the window and I don't want that. The user can control the window size through the settings (maxSearchResultCount and windowMaxHeight).

mkkeck · 2018-10-27T10:12:43Z

Have you seen my fix in dev? There I am setting reiszable to true, resize the app window and at the end I set resizable to false. With this fix, the app behaves like it should.

oliverschwendener · 2018-10-27T11:20:04Z

Didn't see that, this is OK.

oliverschwendener · 2018-10-28T16:16:46Z

src/ts/operating-system-settings/windows/windows-system-commands.ts

@@ -25,7 +25,7 @@ export const windowsSystemCommands: OperatingSystemCommand[] = [
        executionArgument: `${WindowsSettingsHelpers.windowsSettingsPrefix}rundll32 user32.dll,LockWorkStation`,
        icon: lockIcon,
        name: "Lock computer",
-        tags: [],
+        tags: ["lock"],


This tag is redundant. The word "lock" is already in the name.

oliverschwendener · 2018-10-28T19:16:23Z

I suggest you split this PR up in multiple but smaller PRs. I'd rather have small diffs to have a better overview.

oliverschwendener · 2018-11-06T08:01:41Z

@mkkeck ?

mkkeck · 2018-11-06T17:36:35Z

@mkkeck ?

Hi,

how can I make this, in smaller PRs?

oliverschwendener · 2018-11-06T19:22:40Z

This pull request is quite big. 49 files and 5400+ lines changed. I would like you to close this pull request and make multiple new ones which are smaller. Maybe one for updating dependencies and one for refactoring/simplifying code, etc. Do you understand what I mean?

mkkeck · 2018-11-22T13:19:27Z

This pull request is quite big. 49 files and 5400+ lines changed. I would like you to close this pull request and make multiple new ones which are smaller. Maybe one for updating dependencies and one for refactoring/simplifying code, etc. Do you understand what I mean?

Yes I understand. But how could i do? Have I build for each change a new branch?

oliverschwendener · 2018-11-22T13:36:36Z

Yes.

mkkeck added 13 commits October 9, 2018 12:02

Better icon for Windows Taskbar

7fd5d20

If taskbar in Windows not in light color, it's difficult to see the icon.

Some code improvements.

c885ba7

Some code improvements.

254a5e6

Signed-off-by: Michael Keck <github@michaelkeck.de>

Some code improvements.

e85cabf

Git ignore: some folders added

817460b

Get Language/Locale from OS

6e2a279

Merge remote-tracking branch 'upstream/master'

6a74d40

Get Language/Locale from OS

f9e7c64

Forgotten class result-search-number

1ba64aa

Forgotten width in result-search-information

0d2a7d7

Cleaned, validated, optimized HTML-Output

46e36bf

Updated Yarn/NPM-Packages

8e31775

Merge remote-tracking branch 'upstream/master'

38b6eb4

oliverschwendener reviewed Oct 23, 2018

View reviewed changes

src/ts/app-config/default-app-config.ts Outdated Show resolved Hide resolved

oliverschwendener reviewed Oct 23, 2018

View reviewed changes

mkkeck added 3 commits October 24, 2018 11:55

added .vscode folder, because it contains the debug configurations

ff7905d

Switched back to original

9d34d59

Prepared for internationalization

c6e08f7

oliverschwendener reviewed Oct 25, 2018

View reviewed changes

Removed not needed libs

a258545

mkkeck closed this Oct 26, 2018

mkkeck reopened this Oct 26, 2018

oliverschwendener reviewed Oct 28, 2018

View reviewed changes

mkkeck closed this Nov 13, 2018

Some improvement and suggestion #65

Some improvement and suggestion #65

Conversation

mkkeck commented Oct 23, 2018

coveralls commented Oct 23, 2018 • edited Loading

Pull Request Test Coverage Report for Build 435

💛 - Coveralls

oliverschwendener commented Oct 23, 2018

Choose a reason for hiding this comment

mkkeck commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkkeck commented Oct 23, 2018

oliverschwendener commented Oct 24, 2018

mkkeck commented Oct 24, 2018

oliverschwendener commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkkeck commented Oct 26, 2018

oliverschwendener commented Oct 26, 2018

mkkeck commented Oct 26, 2018 • edited Loading

oliverschwendener commented Oct 26, 2018

mkkeck commented Oct 26, 2018 • edited Loading

mkkeck commented Oct 26, 2018 • edited Loading

oliverschwendener commented Oct 26, 2018 • edited Loading

mkkeck commented Oct 26, 2018

oliverschwendener commented Oct 26, 2018

mkkeck commented Oct 26, 2018

oliverschwendener commented Oct 26, 2018

mkkeck commented Oct 26, 2018 • edited Loading

oliverschwendener commented Oct 27, 2018

mkkeck commented Oct 27, 2018

oliverschwendener commented Oct 27, 2018

Choose a reason for hiding this comment

oliverschwendener commented Oct 28, 2018

oliverschwendener commented Nov 6, 2018

mkkeck commented Nov 6, 2018

oliverschwendener commented Nov 6, 2018

mkkeck commented Nov 22, 2018

oliverschwendener commented Nov 22, 2018

coveralls commented Oct 23, 2018 •

edited

Loading

mkkeck commented Oct 26, 2018 •

edited

Loading

mkkeck commented Oct 26, 2018 •

edited

Loading

mkkeck commented Oct 26, 2018 •

edited

Loading

oliverschwendener commented Oct 26, 2018 •

edited

Loading

mkkeck commented Oct 26, 2018 •

edited

Loading