Skip to content
This repository was archived by the owner on Jan 14, 2023. It is now read-only.

Conversation

@rNoz
Copy link
Contributor

@rNoz rNoz commented Dec 29, 2019

Some features and bugfixes presented in #12 (tray show/hide, resume/pause form tray, durations, confirmations, login mode, ...)

Feel free to modify/review everything as you want.

I didn't modify the version nor README/about window. Our team will use this PR in production during the next days. In case we find something we will notify you.

rNoz added 17 commits December 22, 2019 19:03
There were errors in Routes like this:

Warning: Failed prop type: Invalid prop `component` of type `object` supplied to `Route`, expected `function`.
    in Route (created by AppView)
    in AppView (created by Context.Consumer)
    in Connect(AppView) (created by Route)
    in Route (created by Routes)
    in Switch (created by Routes)
    in Routes
    in Router (created by HashRouter)
    in HashRouter
    in Provider
    in ThemeProvider

Bugfixed doing what is said here: remix-run/react-router#6471
…) and hours (float) allowed when editing TimeEntryModal. Added support for right slot in Label. Conversion between duration/hours. Form fields checked individually (onBlur). Refactored validator for Update/Publish. Package: @hapi/joi updated to v16. Tests added/passed.
…ied or initial state is volatile (cames from Timer). Reached a snapshot bug in one test (disabled).
…eature: tray supports pause/resume current timer.
@rNoz
Copy link
Contributor Author

rNoz commented Dec 31, 2019

So far, the only "trouble" is the tray icon in electron under Linux, not working as expected.

To repair it, I test many solutions, until I found the simplest one: including the icon in the asar package. There are many issues regarding this problem in the official electron repo. Avoid the electron v8 because it seems present again.

Also, I updated to the latest stable version the electron package (7), in case you are interested. Everything worked as expected, so, if I were you, I will move to 7 due to optimizations.

I can update the PR if you want, if not, these are the only changes (I didn't try in Mac/Win):

@@ -34,6 +34,7 @@
     "files": [
       "common/**/*",
       "main/**/*",
+      "assets/**/*.png",
       "dist/**/*",
       "node_modules/**/*"
     ],
@@ -76,7 +77,7 @@
     "babel-plugin-styled-components": "^1.10.0",
     "cross-env": "^5.2.0",
     "css-loader": "^2.1.0",
-    "electron": "^5.0.2",
+    "electron": "^7.1.7",
     "electron-builder": "^20.41.0",
     "electron-react-devtools": "^0.5.3",
     "enzyme": "^3.9.0",

rNoz added 5 commits January 3, 2020 09:31
… Bugfixed: tray icon needed png icons in asar file.
… time paused when system is idle, optional discard idle time from timer, notifications and temporary comments in timer.
…are the same node version to avoid recompile modules on each mode.
…and contributors added. Optimized AUR package in support dir.
@Spring3 Spring3 self-requested a review January 4, 2020 15:05
rNoz added 2 commits January 4, 2020 16:33
… when quitting the app with temporary data. Better script to build the AUR package.
echo "updated ${dir}/PKGBUILD with version ${pkgver} and checksum ${md5}"
}

function fn_makepkg {
Copy link
Owner

Choose a reason for hiding this comment

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

I assume that you built the image locally on your machine, right?
For now build for win and ubuntu is done automatically on merge to master. This is done using the electron-builder package, which automatically upload the binaries to the /releases section. It also handles auto-update once the application is started.
For macOS, however builds are done locally cause it needs to be signed and it can only be done from an image / machine, running on macOS. I played with macOS machines that circleCi has, but it didn't work with the keychaining and key generation.

What about this? Will it support auto-update feature? Can it be run automatically from CircleCI (I mean, it can, but will it require any different or specific steps, ot it'll work following the electron-builder's logic?)

Copy link
Owner

@Spring3 Spring3 left a comment

Choose a reason for hiding this comment

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

I've went through the code and everything looks pretty much fine. I decided to not mention any difference in linting style that we use, as well as some minor things like checking for an array with variable instanceof Array instead of Array.isArray(variable) and so on.

I've also seen some of your comments in code about the issues that you have detected and as well that you were only able to check the app on linux. I will need to set up the dev env, cause I don't have access to an exising redmine instance anymore, so I will have to use docker for that and configure projects form scratch.

TLDR; I will try to check and fix what you have mentioned, tests included, see if it works on other OS'es and then will get back to you here @rNoz .

@rNoz
Copy link
Contributor Author

rNoz commented Jan 7, 2020

Thank you @Spring3. Great news. It is your project, we are just contributing :), so feel free to modify everything you want (styles, code, features, bugs, etc). We cannot ensure you to be bug-free or 100% optimized code, since we are new with React/Redux, but we tried our best. So, be ready to receive bugfixes if we find anything ;)
We are using it (v1.1.0) everyday. So far, everything cool, but we work mainly with Ubuntu/Arch Linuxes.
Have a good day.

@rNoz
Copy link
Contributor Author

rNoz commented Jan 8, 2020

Sort of "bug" found: when suddenly quitting/closing the system, and the app is running (play), it doesn't save the current duration/comments.

Because this is completely related with the Timer, and it is the most "complex" component due to the weird cases (connection with IPC-main process, intervals/timeouts, react and window events), I would like to ask you. In theory, it should be controlled, because the process should call "close" (window, main/index), beforeunload (Timer window event) and even before unmount (react event).... but now it is not storing the current time.

Any idea? The problem with this "bug" is that is quite slow to eval-repair, because apparently, it is not the same quitting the app than switching off the computer.

The only workaround I can think of is simply storing every X minutes the current data. Those are the minutes we are able to loss for this corner case (eg. 5 minutes of tracking). Do you suggest another acceptable solution?

It is not a big deal, but it's true as an user I would like to just trust the tracking of time of redshape without corner cases. And if we are continuously working with redshape we would loss a bit, but if we just use every few hours, we can loss much time (how we discovered this bug today). And that is not affordable.

@Spring3 Spring3 changed the base branch from master to feature/rnoz-contributions January 23, 2020 20:10
@Spring3
Copy link
Owner

Spring3 commented Jan 23, 2020

Merging into a separate branch since I will need to check the functionality on macos and also becasue merge to master triggers automatic release. Thank you again for your contributions @rNoz

@Spring3 Spring3 merged commit f351995 into Spring3:feature/rnoz-contributions Jan 23, 2020
@rNoz
Copy link
Contributor Author

rNoz commented Jan 24, 2020

I have new contributions that I worked during this week. Everything is finished, but I am leaving a few days for testing in production. Tomorrow morning I will push to my repo.
One last question. The company at the end allowed me to work a whole month on this. I am really happy about that, but it is more than the expected (initially was only 1 week). I suppose they will promote this project eventually, are you happy with that? Showing you as the author and the company efforts as major contributor. We integrated so many features, UX, bugfixes that we are happy with our efforts and we would like to talk some day about it.
Thanks to you for starting this project :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants