w8jcik commented on a change in pull request #334:
URL:
https://github.com/apache/incubator-nuttx-apps/pull/334#discussion_r455143417
##########
File path: graphics/lvgl/Makefile
##########
@@ -78,20 +78,21 @@ $(LVGL_UNPACKNAME): $(LVGL_TARBALL)
$(Q) mv lvgl-$(LVGL_VERSION) $(LVGL_UNPACKNAME)
$(Q) touch $(LVGL_UNPACKNAME)
-lvgl/lvgl.h: $(LVGL_UNPACKNAME)
+$(LVGL_UNPACKNAME)/lvgl.h: $(LVGL_UNPACKNAME)
-$(APPDIR)/include/graphics/lvgl.h: lvgl/lvgl.h
- @echo "CP: lvgl/lvgl.h"
- $(Q) cp lvgl/lvgl.h $(APPDIR)/include/graphics/lvgl.h
+exports/graphics/lvgl.h: $(LVGL_UNPACKNAME)/lvgl.h
+ $(Q) mkdir -p exports/graphics
+ @echo "CP: $(LVGL_UNPACKNAME)/lvgl.h"
+ $(Q) cp $(LVGL_UNPACKNAME)/lvgl.h exports/graphics/lvgl.h
Review comment:
Hi @v01d the prefix is already present, because currently LVGL wrapper
is copying some header to `include/graphics` to be consistent with other NuttX
apps libraries. Change suggested by @xiaoxiang781216 removes this prefix. I was
asking should we really remove it, because this way we break compatibility and
violate consistency. Breaking compatibility is fine for me. But what is the
official guideline here. Do we want `graphics` in front of all LVGL includes
because it sits in graphics? Right now it seems to be in front of one of the
headers and probably not in front of the others. I think it should be still
possible to make `#include <graphics/lvgl/...>` work. The question is do we
want it.
Update: Heh, I was slow, two answers appeared in the meantime :) Yup
@acassis, I also reminded myself branding change when writing this. Ok
@xiaoxiang781216, I am going with making the life of programmer easier.
@acassis in this repository only lvgldemo, but maybe users do :D
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
