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

Desktop: add checkmark to menu item, if Dev Tools are on #1949

Merged
merged 3 commits into from
Oct 9, 2019

Conversation

tessus
Copy link
Collaborator

@tessus tessus commented Oct 4, 2019

If one closed the dev tools window, one could think that the Dev Tools are closed.
This is not the case. After the next action, the window opens again. This can be
confusing, therefore I added a checkmark to the menu item so that it is very clear.

If one closed the dev tools window, one could think that the Dev Tools are closed.
This is not the case. After the next action, the window opens again. This can be
confusing, therefore I added a checkmark to the menu item so that it is very clear.
@tessus tessus added the desktop All desktop platforms label Oct 4, 2019
@laurent22
Copy link
Owner

I think it won't work if the tools are opened via the command line?

Also the code doesn't build due to this issue: https://eslint.org/docs/rules/no-case-declarations So you'll need to wrap the case in brackets, as I've done for the case statement above yours for example.

@tessus
Copy link
Collaborator Author

tessus commented Oct 4, 2019

I think it won't work if the tools are opened via the command line?

Most likely not, because I haven't looked at it. 2 questions:

  • I thought that command line arguments are not supported anyway
  • how can I open it via the command line? (can you point me to the code?)

Also the code doesn't build due to this issue:

Hmm, this is odd. Did you change the linter? I will run the npm install again just to make sure.
I tested the code locally and it works splendidly.

@laurent22
Copy link
Owner

Most likely not, because I haven't looked at it. 2 questions:

* I thought that command line arguments are not supported anyway

* how can I open it via the command line? (can you point me to the code?)

It's mentioned in https://joplinapp.org/debugging but I guess it's not a big issue if it's not supported.

Hmm, this is odd. Did you change the linter?

No, I think that rule was there since the beginning, but maybe for some reason the pre-commit hook doesn't work for you.

@tessus
Copy link
Collaborator Author

tessus commented Oct 5, 2019

It's mentioned in https://joplinapp.org/debugging but I guess it's not a big issue if it's not supported.

I can try to make it work. Not sure how complicated it will be.

No, I think that rule was there since the beginning, but maybe for some reason the pre-commit hook doesn't work for you.

Hmm, the linter usually runs. Very strange. Maybe I was testing something (remember the issue we had with linter changes not being applied) that interfered with it. No matter, I've re-run npm install and it fixed it.

@tessus
Copy link
Collaborator Author

tessus commented Oct 6, 2019

I just looked into the command line argument again. This is kind of a strange situation.

e.g. when you run ./run.sh, it actually starts with --open-dev-tools. Even if you start the dev mode manually without the argument, it still starts the dev tools, because the code also checks, whether or not you run in dev mode. And no matter the argument, when the app is in dev mode, the dev tools are opened.
The stange thing is however that when using the menu item, a second instance of the dev tools is opened. This was always the case and has nothing to do with this PR.

So the problem is that 2 different instances of the dev tools are opened. One instance for the menu item and a separate instance for the command line option. And they have nothing in common.
(e.g. I can't close the dev tools via the menu item, when the app was started with that command line option)
Therefore I doubt that it is possible to actually change the menu item for the command line argument.

Update: But this PR was intended to minimize the confusion when using the app in normal production use. Not when debugging or running in dev mode.

@tessus
Copy link
Collaborator Author

tessus commented Oct 8, 2019

@laurent22 did you see my last comment?

@laurent22
Copy link
Owner

Yes, I think it's fine as those flags are mostly used only for development, we can merge it once the reducer issue is fixed.

@tessus
Copy link
Collaborator Author

tessus commented Oct 8, 2019

Which reducer issue?

@tessus
Copy link
Collaborator Author

tessus commented Oct 8, 2019

If you are talking about the 2 instances of the dev tools this has nothing to do with this PR. This also happens without this PR. Or did you mean the linter problem. I fixed that 3 days ago.

@laurent22
Copy link
Owner

Sorry it looks like GitHub changed something to the review comments. Instead of submitting them it seems they are in a draft state, so I can see them but you can't. I'm going to submit them now (it's a bit confusing since the submit button is above the comment below an empty text area).

ElectronClient/app/app.js Outdated Show resolved Hide resolved
@laurent22
Copy link
Owner

Looks good now, thanks @tessus!

@laurent22 laurent22 merged commit 0829552 into laurent22:master Oct 9, 2019
@tessus tessus deleted the feature/dev-tools-toggle-state branch October 12, 2019 00:47
scoroi pushed a commit to scoroi/joplin that referenced this pull request Nov 10, 2019
)

* add checkmark to menu item, if Dev Tools are on

If one closed the dev tools window, one could think that the Dev Tools are closed.
This is not the case. After the next action, the window opens again. This can be
confusing, therefore I added a checkmark to the menu item so that it is very clear.

* fix no-case-declarations

* fix reducer issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants