search

quelea-projection / Quelea

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

Modernization of remote control web app #369

Open micahlt opened 3 years ago

micahlt commented 3 years ago

Just fiddling around with the web remote control and I noticed a few minor layout issues. I went to go take a look at the source code and I have a few quick questions:

If this is just a matter of out-of-date source code, I'd love to help refactor the remote webapp since it's one of my favorite parts of Quelea. Love what you guys are doing!

ArvidNy commented 3 years ago

I think Ben wrote the original code many years ago, so he could probably give you a better answer, but my guess is that most of it is just a matter of out-of-date code. I've updated the code since then a few time, but I did not know about the Fetch API nor template literals. As for the style sheet, I suspect that it was originally just a matter of not having to add more contexts to the remote server. Not sure performance-wise what's the better option, but I'd be open for moving it to a separate CSS.

Sounds good to me if you want to go through the code and refactor it!

micahlt commented 3 years ago

Awesome! Is there any downside or performance loss that would make adding a context to the server negative? I'm primarily a Javascript dev, so I know very little about Java.

beng92 commented 3 years ago

I did originally write this code a long time ago building on Michael's proof of concept at the time. I had very little understanding of javascript at the time, and it proved to be quite difficult to maintain simplicity and speed. There are likely new technologies existing now that did not then, or that I was unaware of.

I have nothing against improvements being made to that section of code,, however I think there needs to be a specific need and goal in mind rather than refactoring for the sake of refactoring.

What new features would your additions create? What bugs or issues would they solve? What performance improvement would it create? Will it support at least 99% of browsers as the current solution (I think it works back to ie6 currently with some minor formatting issues due to CSS, and when we collected data at the time this was a concern)

berry120 commented 3 years ago

The issue with switching to more modern JS (such as fetch, template literals etc.) is that by doing so we're breaking compatibility with older browsers, most notably IE11 (which of course no-one should be using now, but that means very little in the grand scheme of things!) I don't think we ever supported IE6, but Ben may be right there as he wrote that original code.

I've no big issue with this personally, but similar to Ben's point, if we're breaking compatibility I think we should have a reason for doing so. If that's the case of fixing a bug, or adding a new feature that can't sensibly be done with the old approach then that's absolutely fine (you mentioned layout issues, so not sure if this comes into it?) - but if it's just a case of making the code look a bit neater then I'm not sure that's a good enough reason in its own right, personally.

In terms of the CSS being bundled in - this was a performance thing since Quelea itself is actually serving the content, so this just means that we remove the need for a separate stylesheet request for each client. Not a big thing, but since it's the desktop application itself that's serving the content, it made sense to keep the performance impact as low as we could.

Certainly don't want to put you off contributing though! If you could expand on the layout issues and that's something you're happy to take a look at and fix, that would be fantastic 😊

micahlt commented 3 years ago

@beng92:

What new features would your additions create? What bugs or issues would they solve? What performance improvement would it create?

As far as features, I think the existing featureset is perfectly sufficient except for the fact that the layout should be mobile-responsive. I'm not aware of any bugs with the existing client besides layout issues, which I'll go into in a second. As far as performance, the Fetch API has been shown to be slightly faster than XMLHttpRequest, though it's a matter of how long it takes to parse JSON. However, as far as I'm aware the client currently uses HTML and not JSON, correct?

Will it support at least 99% of browsers as the current solution (I think it works back to ie6 currently with some minor formatting issues due to CSS, and when we collected data at the time this was a concern)

According to caniuse, the XMLHttpRequest API only has 97.18% support across browsers. Meanwhile, the Fetch API has around 96.26%, making it less than one percent more unsupported. At the moment, IE6-11 only makes up less than a percent of all current browsers. However if we're still interested in supporting IE11 (at 0.76% market share), there are several polyfills that could be loaded if the browser did not support Fetch.

@berry120:

if it's just a case of making the code look a bit neater then I'm not sure that's a good enough reason in its own right

I'd respectfully disagree with that - making the code cleaner helps out contributors and makes feature additions and edits much easier. Also, from a nit-picky performance standpoint, since the Fetch API is much more concise than XMLHttpRequest, you're technically saving a few bytes from being loaded 😄

Not a big thing, but since it's the desktop application itself that's serving the content, it made sense to keep the performance impact as low as we could.

No problem at all - totally understand that, UX for the actual application has to be the priority.

If you could expand on the layout issues and that's something you're happy to take a look at and fix, that would be fantastic 😊

Of course! A few different things:

The rest of the "layout issues" are really just my pet peeves about uneven button sizing and just outdated UI, but I suppose that's for another time. And if I seem too harsh or particular, sorry 😞 I'd just love to see this section of Quelea improved.

ArvidNy commented 3 years ago

However, as far as I'm aware the client currently uses HTML and not JSON, correct?

That's true. I actually started looking at creating a JSON server instead (which would make app creation a lot simpler) but I haven't finished it yet. First it was just a matter of looking at making a HTTPS server instead, since neither Apple nor Google seem to appreciate apps making non-secure HTTP requests (even if it's just a local request). At some point I'll try to finish that and make it a PR.

We've actually already merged my PR #202 that addresses some of those issues and makes the UI look and function more similar to the Android/iOS app. Not sure what I did with the logout button when I look at the screenshots, but if I accidentally removed it I'd be open to add it back in. You should try the 2021.0 beta release and see if that works closer to what you expected.

With that said, if you have any suggestions to the new UI I don't mind the input!

micahlt commented 3 years ago

Ah, okay! I didn't realize that 2021.0 had been released already. After installing and trying it, it seems like you fixed most of the issues I was referencing. Yep, it does look like the logout button has been completely removed. As far as further suggestions, it looks like the hamburger menu button overlaps the search button when the menu is open. I might also in general shrink the icons a little to allow some more padding. The search layout is slightly confusing as well, and could be improved. However, do these thoughts need to go into a separate issue?

berry120 commented 3 years ago

I'd respectfully disagree with that - making the code cleaner helps out contributors and makes feature additions and edits much easier.

Sure, it does - but the flip side is that any change introduces risk, and potential incompatibility when we're talking about the frontend - so I've become increasingly cautious over the years :-) I'm all for making code cleaner & more readable alongside other tasks that's also touching the same area of code, but I've seen plenty of cases where (what should have been) purely stylistic changes have broken things functionally too.

That being said, I think moving things over to JSON & secure endpoints to keep the apps happy could well be a significant enough change to justify switching to a more modern frontend at the same time, so if it's included as part of that work I'm all for it 👍

2021.0 is nearly there, there's one more issue in particular I need to look at in the beta before we move to a second beta, then hopefully a final release shortly afterwards. Just a case of finding the time at the moment, but my aim is still to get it out by the end of August! (Then again, it was by the end of June, so take from that what you will.)

micahlt commented 3 years ago

Interesting - so any work on this should really be done post-2021.0 it seems. I'm definitely interested in helping move to a more modern frontend though!

berry120 commented 3 years ago

By all means start work by forking it, we just wouldn't merge it yet is all.

On Sun, 15 Aug 2021, 01:49 Micah Lindley, @.***> wrote:

Interesting - so any work on this should really be done post-2021.0 it seems. I'm definitely interested in helping move to a more modern frontend though!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/quelea-projection/Quelea/issues/369#issuecomment-898977502, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZ6JDNTTGYG5L3QZESHX6LT44FI7ANCNFSM5BPNAKXQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .