rschroll / beru

The Basic Epub Reader for Ubuntu
http://rschroll.github.io/beru/
GNU General Public License v3.0
25 stars 12 forks source link

Review changes made for Issue #53 CMake #54

Closed randy-jr-olive closed 10 years ago

randy-jr-olive commented 10 years ago

I've made all the CMakeList files the project needs. I managed to get the project to build, compile and install. However, when I try to run the qmlscene ui/main.qml, it can't seem to find the directories where the plugins are installed. Could you have a look?

rschroll commented 10 years ago

Hey -- looking good. I've managed to get it sorta working, which is a good sign. One thing to note is that you have to run qmlscene with the -I argument to tell it where to look for the plugins. Because of the way I've set things up to work around stupidities of qmlscene's argument handling, you also need to pass an --appargs argument. So your launch command needs to be qmlscene -I . ui/main.qml --appargs="".

When I do that, I get almost all the plugins to load. There's a problem with the HttpServer, which is due to an inconsistency in the naming. (My fault, not yours.) As I go through the changes, I'll point that out.

Because of many arguments you need to give qmlscene, I've included a shell script, beru, that takes care of things. It also ensures that it can be run regardless of the CWD. We should get this working with CMake, either by copying it to the build directory or by rewriting it to point to the build directory instead of the source directory.

I also notice that a bunch of the HTML resources don't get copied over the build directory. This is probably just a simple oversight.

Also, Texture.qml doesn't get built or copied over, but we were expecting that. Just do it by hand to test, and I'll see if I can work out how to make it happen in CMake.

All the .pro files should be deleted.

I'm going to make a bunch of comments on the commits themselves. I'm overly fussy about some things. Sorry.

rschroll commented 10 years ago

You can make the changes in new commits and let me squash them appropriately, or you can do the history rewriting yourself. Whichever you prefer.

In the future, you probably want to make a separate branch for each pull request. This let's you avoid extra merges or rewriting history on your master.

rschroll commented 10 years ago

Sorry I keep hitting you with all these little details. While I've used CMake before, this is the first time I've actually tried to understand what it's doing and how it's set up. So I'm being really picky in the process.

I really appreciate the work you've put in here. Figuring out where to begin is the hardest and most important part, and I didn't want to do it. :)

randy-jr-olive commented 10 years ago

Ok, so now all the changes are done for this request, how do I send them to you? new pull?

rschroll commented 10 years ago

Nope, we stay right here. Github is smart enough to figure out that new commits to an existing branch with a pull request belong there. If you check out the "Files changed", you'll see it includes your new changes. All the comments on old code are hidden, so we can focus on the new stuff.

randy-jr-olive commented 10 years ago

Wow, what an age we live in. This is super cool. Ok, then thanks for reopen, I clicked "close and comment" not knowing what it did.

randy-jr-olive commented 10 years ago

"In the future, you probably want to make a separate branch for each pull request. This let's you avoid extra merges or rewriting history on your master." I'm new to this master branch trunk stuff. Can you describe a brief workflow (how to start, work and submit to a github project). I've watched a few how-to videos, but you're input would be nice too.

randy-jr-olive commented 10 years ago

I've tried running the app this morning and I'm getting an error: 27085 Segmentation fault (core dumped) QT_SELECT=5 qmlscene -I "$DIR" "$DIR/ui/main.qml" --appargs="$*"

Is this happening to you? I think it's qmlscene that is crashing when it tries to run the main.qml file, and I also think it might just be my local setup here.

randy-jr-olive commented 10 years ago

while debugging, I noticed this: QQmlImports(Server.qml)::importExtension: loaded "Epub/qmldir" beru: line 3: 2818 Segmentation fault (core dumped) QT_SELECT=5 qmlscene -I "$DIR" "$DIR/ui/main.qml" --appargs="$*"

It fails to load right after trying to load the epubreader plugin. Not sure why, still investigating.

randy-jr-olive commented 10 years ago

Ok, so, to get this to work for me, I DID need to include all the files that are to be used to build a plugin, even if in the header and source files they are imported. I've changed my CMakeLists.txt to reflect this, and I've got a fully working, CMake project on my local. commits to follow.

rschroll commented 10 years ago

Thanks for tracking that seg fault down. I don't understand why some of those files have to be included, but I'm not really curious enough to learn why. I've squashed much of this into a single commit, since I don't want non-building commits in master, if possible. A few changes:

If there's anything else that didn't get included, please open another pull request. I probably just overlooked it.

Thanks again for all your work on this. It was not an easy place to begin, I know!

randy-jr-olive commented 10 years ago

No worries, glad to help, it's something I needed to do before I started testing the app on my devices anyways. I didn't know anything about CMake before I began, and now at least I know what it does. I'll keep an eye on tweaking the CMake to work better with Qt creator, I liked how easy the .pro files were.