search

HEnquist / camillagui

GNU General Public License v3.0
9 stars 1 forks source link

Typescript migration & GUI overhaul #24

Closed JWahle closed 3 years ago

JWahle commented 3 years ago

This includes the commits for PR #22.

I converted the sidepanel and devices tab to typescript and simplified the event handling for both. I also restyled the devices tab and fixed some layout issues.

If you are fine with these changes, I would continue to do similar things with the filters, mixers and pipeline tabs.

HEnquist commented 3 years ago

This looks great! I haven't looked into the details of the changes, but the overall ideas make a lot of sense and clean things up nicely. Do you think it makes sense to merge this now, or would it be better to do it after the whole thing has gotten the same treatment? I'm also starting to feel that my lack of time for gui-stuff might be holding you back.. maybe we should think of something more efficient (?).

JWahle commented 3 years ago

Since I touch a lot of code and thus get a couple of merge conflicts, during the typescript migration when working on other things, I prefer merging sooner than later. (So PR #22 first, then this one)

If you'd like to give me write access to the CamillaGUI repos, I'd of course use it responsibly, but there's no need for that on my part. Until now, I had to do a handful of small merges, but nothing serious - yesterday was the first time I had to spend about 10 minutes on merging, so for me this is totally acceptable. Once the parts that I work on are converted to typescript, this will probably be not happen again. I think, for an open source project, your response times are really fast and I understand that you have other stuff to do and cannot respond or merge immediately. Unless you feel uncomfortable having to look at this GUI stuff on a regular basis, I see no problem here :)

HEnquist commented 3 years ago

Making you a collaborator on the repos is certainly one possibility. If we work that way, we should anyway make changes in a new feature branch and then merge that to master, and if we always review each others stuff before merging the quality will likely go up. Would you be interested in this?

JWahle commented 3 years ago

Sounds good, but doesn't this mean, things stay as they are? Unless you intend to do more GUI stuff. ;)

HEnquist commented 3 years ago

It's not a very big change in practice no :) But I think having that extra step is worth the hassle. I don't think we need to review every changed character with a magnifying glass, just look over the code and check that for example the help text (readme, tooltip etc) for new things makes sense. That kind of mistake is very easy to do, and also very easy for someone else to spot.

JWahle commented 3 years ago

I'm a big fan of code reviews - so this is totally fine with me :)