Skip to content

Rename ArtNet, Artnet, artnet, Art-net to Art-Net - #1405

Merged
peternewman merged 5 commits into
OpenLightingProject:masterfrom
nils-van-zuijlen:rename-to-Art-Net
May 5, 2018
Merged

Rename ArtNet, Artnet, artnet, Art-net to Art-Net#1405
peternewman merged 5 commits into
OpenLightingProject:masterfrom
nils-van-zuijlen:rename-to-Art-Net

Conversation

@nils-van-zuijlen

Copy link
Copy Markdown
Member

In all comments, strings and documentation.

#1328

In all comments, strings and documentation.

Closes OpenLightingProject#1328
@peternewman

Copy link
Copy Markdown
Member

Amazing thanks @nils-van-zuijlen

Comment thread plugins/artnet/ArtNetNode.cpp Outdated
} else {
OLA_WARN << "ArtNet RDM request failed with code " << reply->StatusCode();
OLA_WARN << "Art-Net RDM request failed with code "
  << reply->StatusCode();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've somehow inserted a Unicode character in here according to the compiler errors.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was an non-breaking space instead of a regular space.

Which prevented proper compiling
@nils-van-zuijlen

nils-van-zuijlen commented Apr 21, 2018

Copy link
Copy Markdown
Member Author

The travis build failed because of this commit in codespell

#1407 should be merged in order to fix this.

@peternewman peternewman added this to the 0.11.0 milestone Apr 22, 2018

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor nits @nils-van-zuijlen . Otherwise LGTM.

Comment thread NEWS Outdated
* Add an option for using the limited broadcast address for ArtNet dmx data
* Fixed Art-Net RDM discovery so DMXWorkshop works correctly - #160
* Fixed a memory leak in the Art-Net RDM implementation
* Add an option for using the limited broadcast address for Art-Net dmx data

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, and not your fault, but could we change dmx to DMX here please.

label {
value: 1
label: "ArtNet"
label: "Art-Net"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These also want updating here, which is where this data is sourced from if you're feeling keen: https://github.com/OpenLightingProject/rdm-app/blob/master/data/pid_data.py#L719

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread plugins/artnet/ArtNetNode.cpp Outdated
// the universe address has changed we need to drop this request
OLA_WARN << "ArtNet Output port has changed mid request, dropping response";
OLA_WARN << "Art-Net Output port has changed mid request, "
<<"dropping response";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a space after << please. Also we'd generally just move the one word that doesn't fit down (i.e. response in this case).

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@peternewman
peternewman merged commit 180d031 into OpenLightingProject:master May 5, 2018
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*
* E131ConfigMessages.proto
* Defines the protocol buffers used to configure the Artnet device

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nils-van-zuijlen Sorry I've just spotted, do you fancy opening a new PR to correct this to E1.31 as it's actually a copy/paste error from then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't it the filename ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the first one should be. I meant line 17 where it talks about using E131ConfigMessages.proto to configure the 'Artnet' device!

nils-van-zuijlen added a commit to nils-van-zuijlen/ola that referenced this pull request May 13, 2018
In plugins/e131/messages/E131ConfigMessages.proto

As requested [there](OpenLightingProject#1405 (review)).
nils-van-zuijlen added a commit to nils-van-zuijlen/ola that referenced this pull request May 13, 2018
In plugins/e131/messages/E131ConfigMessages.proto:17
And libs/acn/E131InflatorTest.cpp:155

As requested there: OpenLightingProject#1405 (review)
@nils-van-zuijlen nils-van-zuijlen mentioned this pull request May 13, 2018
@nils-van-zuijlen
nils-van-zuijlen deleted the rename-to-Art-Net branch July 19, 2018 10:12
@peternewman peternewman mentioned this pull request Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants