Opened 7 years ago

Closed 6 years ago

#9604 closed enhancement (fixed)

PatchBay update for Haiku

Reported by: Pete Owned by: phoudoin
Priority: normal Milestone: R1
Component: Applications Version: R1/alpha4.1
Keywords: Cc:
Blocked By: Blocking: #9159
Has a Patch: yes Platform: All

Description

I've generated a new version of PatchBay, edited to conform (hopefully!) to Haiku coding standards, and using Haiku's ToolTip mechanism rather than the custom code in the original.

I've added a vector icon for the "Unknown Devices" it displays, and given it a Desktop icon as well.

This ticket is a follow-on from #9159, but following Philippe's suggestion there I've started a new ticket.

As noted there, this code should be in .../apps rather than .../tests/... so if it's OK, can someone please move it there in the repository?

Attachments (4)

patchbay.diff (77.1 KB ) - added by Pete 7 years ago.
Updates to the original PatchBay app
0001-PatchBay-revisions-for-Haiku.patch (103.0 KB ) - added by Pete 7 years ago.
git format-patch version of updates
PatchBay_icons.zip (10.9 KB ) - added by Pete 7 years ago.
I-O-M files of the PatchBay icons
0001-additional-style-update-to-patchbay.patch (11.8 KB ) - added by Pete 6 years ago.
2nd attempt to bring code up to style guide

Download all attachments as: .zip

Change History (21)

by Pete, 7 years ago

Attachment: patchbay.diff added

Updates to the original PatchBay app

comment:1 by Pete, 7 years ago

Has a Patch: set

comment:2 by waddlesplash, 7 years ago

Two questions:

  • Will PatchBay appear in Preferences menu after this patch is applied and source moved to .../apps?
  • Can you compile it on GCC4 after this? If not, could it be fixed at the same time?

Thank you!

comment:3 by phoudoin, 7 years ago

Blocked By: 9159 added

comment:4 by phoudoin, 7 years ago

Blocked By: 9159 removed
Blocking: 9159 added

comment:5 by phoudoin, 7 years ago

Thanks Pete for the (git format-)patch. After a 2 years break, I'm setting up a new Haiku system (just bought a new Asus Vivobook S300CA) and hope to be able to work again on Haiku stuffs this week. My coding style guide skills are a bit rusty, too. So, stay tuned.

One remark/question : should we keep the x-vnd.Be.PatchBay signature (for compatibility sake, some existing midi apps may launch it?) or endorse it has our now, x-vnd.Haiku.PatchBay ?

Seems that there is now an app_name_catalog_entry resource to set up too for application name localisation:

resource app_name_catalog_entry "x-vnd.Haiku-PatchBay:System name:PatchBay";

in reply to:  description comment:6 by bonefish, 7 years ago

Replying to Pete:

As our patch guidelines say and people have written on the list, please use git format-patch to create the patch. This will cause your contribution to be credited correctly and since the patch will already include the commit message, it is also less work for the committer.

As noted there, this code should be in .../apps rather than .../tests/... so if it's OK, can someone please move it there in the repository?

Please just do it yourself (in another commit). Unlike subversion, git has no trouble conveying such a change via a patch.

by Pete, 7 years ago

git format-patch version of updates

by Pete, 7 years ago

Attachment: PatchBay_icons.zip added

I-O-M files of the PatchBay icons

comment:7 by Pete, 7 years ago

OK, I've made a git format-patch version of the updates which seems to be correct (moved from src/tests/kits/midi to src/apps). I've also attached a zip of the I-O-M files I created for the app's icons, as they're otherwise only on my machine.

comment:8 by korli, 7 years ago

Pete, do you intend to fix the code style issues in another patch? We have a style checker script in src/tools/checkstyle to help for the task.

in reply to:  8 comment:9 by Pete, 7 years ago

Replying to korli:

Pete, do you intend to fix the code style issues in another patch? We have a style checker script in src/tools/checkstyle to help for the task.

Eh...? I thought that was what I had done... (:-/) (The first thing I did was to run the checker! ... before doing the other rewriting -- which I thought adhered to the style codes,)

comment:10 by korli, 7 years ago

OK, a few things:

  • #ifndef _EndpointInfo_h, _EndpointInfo_h is usually uppercased, dunno if it's in the coding style guidelines..
  • In header files, there is no blank line between license text and header guard.
  • there are a few occurrences of braces on their own lines along with a for or a if.
  • the copyright license header doesn't follow the guidelines.
  • "if (box)" should be "if (box != NULL)"
  • "if (producer) producer->Release();" should be on two lines
  • separate #include group with a newline

Improvements to the checkstyle script are welcome too :)

in reply to:  10 comment:11 by Pete, 7 years ago

Replying to korli:

OK, a few things:

  • #ifndef _EndpointInfo_h, _EndpointInfo_h is usually uppercased, dunno if it's in the coding style guidelines..

Oops, yes -- shouldn't have missed that...

  • In header files, there is no blank line between license text and header guard.

OK

  • there are a few occurrences of braces on their own lines along with a for or a if.

OK

  • the copyright license header doesn't follow the guidelines.

Would you like to expand on that? I tried to figure out what was appropriate, and that was my best guess.

  • "if (box)" should be "if (box != NULL)"

Missed that too. [Maybe because that's one of the guidelines I don't agree with! (:-/)]

  • "if (producer) producer->Release();" should be on two lines

OK

  • separate #include group with a newline

OK

Improvements to the checkstyle script are welcome too :)

Yep... None of the above are caught by checkstyle.py. I'm probably not the one to make any improvements, though. (:-))

It really is hard for someone who normally uses a different style to notice all the variances in a large swathe of code, but I'll do my best.

by Pete, 6 years ago

2nd attempt to bring code up to style guide

comment:12 by Pete, 6 years ago

Sorry -- I did these code updates a month ago, but got diverted and never submitted them here. New patch (additional to the previous, not a replacement) attached.

I've tried to address all the style-guide concerns I could find, previously mentioned and unmentioned, except for the Copyright header -- where I have no idea what should be fixed.

Be glad if it can get committed now.

comment:13 by Pete, 6 years ago

Can someone please commit this, so it can be made available as an optional package? It's been three months now... Thanks.

comment:14 by phoudoin, 6 years ago

Owner: changed from nobody to phoudoin
Status: newin-progress

comment:15 by Pete, 6 years ago

Philippe, thanks for taking this on.

I was going to look into whether it would be better as a bundled app or an optional package. I remember you prefer the former (https://dev.haiku-os.org/ticket/9159#comment:11), and I think I agree -- it's only ~80kB.

I'd provide a patch to add it to the generated set, but I can't quite figure out where that is controlled. It's in the .../src/apps/jamfile, but that doesn't seem to cause it to be compiled or added to the image (similarly with a few others, like 3DMov). Where is this set?

comment:16 by phoudoin, 6 years ago

In build/jam/Haiku[64]Image, add PatchBay to SYSTEM_DEMOS variable.

Last edited 6 years ago by phoudoin (previous) (diff)

comment:17 by phoudoin, 6 years ago

Resolution: fixed
Status: in-progressclosed

Patches applied and PatchBay promoted to Haiku default image's system demos in hrev45956.

Note: See TracTickets for help on using tickets.