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

Fix bug 163 - Automatic startup on Windows and Linux #192

Merged
merged 12 commits into from Mar 2, 2018
Merged

Fix bug 163 - Automatic startup on Windows and Linux #192

merged 12 commits into from Mar 2, 2018

Conversation

b00f
Copy link
Contributor

@b00f b00f commented Jan 21, 2018

Changes:
fix bug #163 (automatic startup)
fix misplacing traffic lights in Windows

  • Works on Windows.
  • Works on Linux.

@theshadowx
Copy link
Collaborator

Could you add in appveyor.yaml in install section (just after the line 48):

- git submodule update --init --recursive

And also in README, could you replace the line 20 by:

$> git clone --recursive  https://github.com/nuttyartist/notes.git

Thanks

PS: i haven't yet tested this feature, but just to have the continuous integration work with your modifications.

@nuttyartist
Copy link
Owner

nuttyartist commented Jan 27, 2018

I currently tested only on Mac and Windows. It worked well on Windows.
Are you sure it's working on Linux or shall I test it also?

On my Mac, it didn't work, and there were also some minor problems regarding code compilation:

  1. Need to add #include <QProcess>.
  2. Inside the function on line 94 you should change dir to appDir , I think.
  3. Also, seems like the ✓ sign on the menu is always toggled on the Mac so it's not clear what's the current state of the menu action (is it on or off). It doesn't happen with the view->Always stay on top menu bar.

Do you have an idea on what needs to be fixed in order for it to work on Mac?

@nuttyartist
Copy link
Owner

So it's working on Windows and Linux.

I'll merge this and create a new issue for automatic startup specifically for Mac.
This is awesome, thanks, Mostafa.

I plan to work a new minor release with all the different bug-fixes/enhancements/new-features from our "In-Between Releases" project then we'll push a release people would be able to get using our automatic updates system.

Oh but before I merge this, @b00f can you do the code changes I mentioned in the comment above for numbers 1 and 2? Then I'll merge.

@b00f
Copy link
Contributor Author

b00f commented Feb 3, 2018

Sure. I will update it.
Since I don't have mac I can't verify them. Can you get project and update it for mac later?

@nuttyartist nuttyartist changed the title Fix bug 163 Fix bug 163 - Automatic startup on Windows and Mac Feb 3, 2018
@theshadowx theshadowx changed the title Fix bug 163 - Automatic startup on Windows and Mac Fix bug 163 - Automatic startup on Windows and Linux Feb 3, 2018
@nuttyartist
Copy link
Owner

Do you mean to ask if I can implement your project and test it on Mac? I can surely test it. Probably not so much implement it. But I'll try to tweak some things here and there to see if I can make it work.

@nuttyartist
Copy link
Owner

Oh, and make sure to include this feature only for Windows and Linux for now.

@theshadowx
Copy link
Collaborator

It's better to create another PR.

@b00f
Copy link
Contributor Author

b00f commented Feb 14, 2018

Is it possible to create new PR from my forked branch (dev)?

@theshadowx
Copy link
Collaborator

@b00f you can create a branch then do your modifications and then submit PR

@b00f
Copy link
Contributor Author

b00f commented Feb 14, 2018

How about this commit I have submitted? =
highlight search

@theshadowx
Copy link
Collaborator

It was a feature request #81 , I haven't tested yet. sure it would be most welcome if it works as described.

@b00f
Copy link
Contributor Author

b00f commented Feb 14, 2018

Please close this pull request. I will create new branch for pinned messages and markdown features.

@b00f b00f closed this Feb 14, 2018
@b00f b00f reopened this Feb 14, 2018
@nuttyartist
Copy link
Owner

What about the "Automatic startup on Windows and Linux"? We need to merge that first.
Have you applied these changes to the code?:

  1. Need to add #include .
  2. Inside the function on line 94 you should change dir to appDir, I think.

If so, I'll merge this PR. For any additional feature it is best to create a new, different PR, so we'll have a good view of each feature/change with the related code. So you should create a new PR for highlighting text when searching for a note (I will be happy to test that) and for other features you're planning to implement.

@b00f
Copy link
Contributor Author

b00f commented Feb 15, 2018

I have updated the above changes. You can check it by syncing the submodules.
I am sorry, this PR get a little bit messy. I will create new branches for my changes.

@nuttyartist
Copy link
Owner

I have checked the changes but it seems like the search with highlighted text is there where it should be on a different PR and the changes in the comment above are not implemented (it's just a little thing).

So just keep only the things related for automatic startup on Windows and Linux and do the little changes in the comment I mentioned and I'll merge. For the other features, it'll be a good approach to create a new PR for each one.

@b00f
Copy link
Contributor Author

b00f commented Feb 22, 2018

I removed highlight method from this PR. Please merge it with --no-ff method to ignore unrelated changes. Thanks

@nuttyartist
Copy link
Owner

nuttyartist commented Feb 25, 2018

The changes below are still missing in qautostart.cpp:

  1. Need to add #include <QProcess>.
  2. Inside the function on line 94 you should change the variable dir to appDir, I think.

@b00f
Copy link
Contributor Author

b00f commented Feb 26, 2018

I think you need to update submodue:
git submodule update --recursive

@b00f b00f merged commit 3978e47 into nuttyartist:dev Mar 2, 2018
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

4 participants