search

quelea-projection / Quelea

Open source projection software for churches.
https://quelea.org
GNU General Public License v3.0
154 stars 145 forks source link

Jessy jp branch for pull req #592

Closed JessyJP closed 1 year ago

JessyJP commented 1 year ago

This is probably my first ever proper pull request, although I did try before. Anyway, just a minor bug fix and extra ignore for the IntelliJ Idea IDE. I hope it gets accepted quick and easy so I can see how it works and send some other pull requests later.

JessyJP commented 1 year ago

I guess the commits could be accepted for now and then come back to it in the future. At some point, a transpose up and down should be included in the stage control.

berry120 commented 1 year ago

Hey, few points here:

I was testing transposing a song from a MIDI controller in Reaper and it kept throwing errors. I am thinking WHY? Then I noticed the call sequence and fixed it to the point of not throwing errors. It worked but it was still a bit strange as it sometimes transposed and sometimes it didn't. Maybe that's related to what you mentioned about "try with resources".

No, the try with resources is a stylistic thing in code - it will essentially flush & close the stream after the code contained within that block is complete, rather than having to call close() manually. The link I pasted should give you some more context there - it's essentially a neater way of achieving the same thing, and also comes with some additional guarantees (for example, the stream will always be closed even if an exception occurs.)

I think it's fine to leave both even if for some it may be redundant.

It's not a case of it being redundant for some - the extra entry would be redundant for everyone, as just specifying .idea/ in .gitignore ignores it in any directory, including Quelea/.idea. So there's never a need to have both (so long as I'm remembering the .gitignore syntax correctly, that is.) If you specified /.idea/ (note the extra slash at the beginning) then that would be specific to the top level directory and so we'd need both - but don't think that's necessary here. Just need to remove that extra entry.

I guess the commits could be accepted for now and then come back to it in the future.

Afraid that's not how the process works :-) This is why we have a review process - to ensure any external code or changes we're accepting is the best possible quality it can be, even if it's just one-liners like the above. In reality "coming back to it in the future" never tends to happen, so the best time to sort these things is now, before we merge.

JessyJP commented 1 year ago

"t's not a case of it being redundant for some - the extra entry would be redundant for everyone, as just specifying .idea/ in .gitignore ignores it in any directory, including Quelea/.idea. So there's never a need to have both (so long as I'm remembering the .gitignore syntax correctly, that is.) If you specified /.idea/ (note the extra slash at the beginning) then that would be specific to the top level directory and so we'd need both - but don't think that's necessary here. Just need to remove that extra entry."

JessyJP commented 1 year ago

I get now what you mean it's the pattern in the file path rather than the path itself. So the Quelea/.idea/* becomes redundant.
So then it doesn't matter where the ".idea/" is located, all files that contain that pattern will be ignored. While "/.idea/" would be absolute path relative(prefixed) but the current root. I checked the documentation and learned something new.

In that case ".idea/" is the best option because it ignores it in both the root and any sub. I mean feel free to edit. You can edit the pull, right?

About the other thing, initially, I used it only because of its API call because the intermediary was translating the calls between midi and this. For me, that API is not needed anymore but will obviously be needed for the remote.... or maybe not. Because the remote should just have a +/- button for transposing. Therefore, 2 new API functions will be needed. Those APIs in turn will call an internal transpose up and down. I guess that still leaves the transpose HTTP api unused.

I don't know how the pull request looks on your end, never "collaborated" per see before. There are some subtleties for me to get up to speed with about that. Also, while I can make it work in java, it might not really be the most java way of it working I have been jumping through too many languages lately (like LUA hahaha). The point is if you see it and can edit it great just make the changes as you see fit or if I am supposed to resubmit an edited version you have to tell me explicitly.

berry120 commented 1 year ago

if I am supposed to resubmit an edited version you have to tell me explicitly.

Yes please :-) You can update your current branch and then it'll update the PR here.

To be clear, as to the try with resources point, you can replace this:

        OutputStream os = he.getResponseBody();
        os.write(response.getBytes(Charset.forName("UTF-8")));
        os.flush();
        os.close();

With just this:

        try(OutputStream os = he.getResponseBody()) {
            os.write(response.getBytes(Charset.forName("UTF-8")));
        }

...and in the other file, delete the superfluous gitignore entry as described above.

JessyJP commented 1 year ago

The changes are done. In the process, there were some syncing peculiarities, which resulted in a bunch of merges. I tried to squash the commits so that they appear as only 2 commits but this was the result in the end. I don't know if that's a problem. There definitely seems to be a delicate order to fetching, squashing, merging, uploading, etc.!

berry120 commented 1 year ago

No problem with multiple commits - the only thing the reviewers (me in this case) really care about is the diff, which all looks good. Commits all get squashed into one anyway.