Implement new timeline with collapsed/hidden tags #4409

dacap · 2024-04-09T21:18:34Z

First progress for #3904 and #920

One commit ("Add support for variable frame width in Timeline") comes from #3583 and we're not sure if it will be finally required.

aseprite-bot

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 48. Check the log or trigger a new build to see more.

aseprite-bot · 2024-04-09T21:30:49Z

src/app/commands/cmd_new_frame_tag.cpp

@@ -52,10 +51,10 @@ void NewFrameTagCommand::onExecute(Context* context)
 frame_t from = reader.frame();
 frame_t to = reader.frame();

- auto range = App::instance()->timeline()->range();
+ view::RealRange range = context->range();


warning: variable 'range' of type 'view::RealRange' (aka 'view::Range') can be declared 'const' [misc-const-correctness]

Suggested change

view::RealRange range = context->range();

view::RealRange const range = context->range();

aseprite-bot · 2024-04-09T21:30:49Z

src/app/commands/collapse_tag.cpp

+
+void CollapseTagCommand::onLoadParams(const Params& params)
+{
+ std::string id = params.get("id");


warning: variable 'id' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change

std::string id = params.get("id");

std::string const id = params.get("id");

aseprite-bot · 2024-04-09T21:30:49Z

src/app/commands/collapse_tag.cpp

+bool CollapseTagCommand::onChecked(Context* ctx)
+{
+ const ContextReader reader(ctx);
+ auto tag = this->tag(ctx);


warning: 'auto tag' can be declared as 'auto *tag' [readability-qualified-auto]

Suggested change

auto tag = this->tag(ctx);

auto *tag = this->tag(ctx);

aseprite-bot · 2024-04-09T21:30:49Z

src/app/commands/collapse_tag.cpp

+{
+ const ContextReader reader(ctx);
+ auto tag = this->tag(ctx);
+ return (tag && tag->isCollapsed());


warning: implicit conversion 'Tag *' -> bool [readability-implicit-bool-conversion]

Suggested change

return (tag && tag->isCollapsed());

return ((tag != nullptr) && tag->isCollapsed());

aseprite-bot · 2024-04-09T21:30:49Z

src/app/commands/collapse_tag.cpp

+
+void CollapseTagCommand::onExecute(Context* ctx)
+{
+ auto tag = this->tag(ctx);


warning: 'auto tag' can be declared as 'auto *tag' [readability-qualified-auto]

Suggested change

auto tag = this->tag(ctx);

auto *tag = this->tag(ctx);

aseprite-bot · 2024-04-09T21:30:50Z

src/app/ui/timeline/timeline.cpp

+ frameBoxWidth());
+}
+
+view::col_t Timeline::getFrameInXPos(const int x) const


warning: method 'getFrameInXPos' can be made static [readability-convert-member-functions-to-static]

Suggested change

view::col_t Timeline::getFrameInXPos(const int x) const

view::col_t Timeline::getFrameInXPos(const int x)

src/app/ui/timeline/timeline.h:349:

- col_t getFrameInXPos(const int x) const; + static col_t getFrameInXPos(const int x) ;

aseprite-bot · 2024-04-09T21:30:50Z

src/app/ui/timeline/timeline.cpp

+ int x = 0;
+ for (; f<nframes; f=col_t(f+1)) {
+ int duration = m_adapter->frameDuration(f);
+ int w = frameBoxWidth();


warning: variable 'w' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int w = frameBoxWidth();

int const w = frameBoxWidth();

aseprite-bot · 2024-04-09T21:30:51Z

src/app/ui/timeline/timeline.cpp

-int Timeline::calcTagVisibleToFrame(Tag* tag) const
+// Returns the last frame where the frame tag (or tag label) is
+// visible in the timeline.
+view::col_t Timeline::calcTagVisibleToFrame(Tag* tag) const


warning: method 'calcTagVisibleToFrame' can be made static [readability-convert-member-functions-to-static]

Suggested change

view::col_t Timeline::calcTagVisibleToFrame(Tag* tag) const

view::col_t Timeline::calcTagVisibleToFrame(Tag* tag)

src/app/ui/timeline/timeline.h:407:

- col_t calcTagVisibleToFrame(Tag* tag) const; + static col_t calcTagVisibleToFrame(Tag* tag) ;

aseprite-bot · 2024-04-09T21:30:51Z

src/app/ui/timeline/timeline.h

@@ -260,16 +290,16 @@ namespace app {
 void handleRangeMouseDown(const ui::Message* msg,
 const Range::Type rangeType,
 doc::Layer* fromLayer,
- const doc::frame_t fromFrame);
+ const col_t fromFrame);


warning: parameter 'fromFrame' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const col_t fromFrame);

col_t fromFrame);

aseprite-bot · 2024-04-09T21:30:51Z

src/app/ui/timeline/timeline.h


 void handleRangeMouseMove(doc::Layer* fromLayer,
- const doc::frame_t fromFrame);
+ const col_t fromFrame);


warning: parameter 'fromFrame' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const col_t fromFrame);

col_t fromFrame);

* Moved app::DocRange to view::Range * Moved range_utils to view/cels.cpp * Moved layer_utils to view/layers.cpp * Created more functions in app::Site to access selected cels (they use the functions from view/cels.h)

) We need an extra code layer (view::TimelineAdapter) to convert between old "real frames" (view::fr_t = doc::frame_t) and new "virtual frames" (view::col_t). These new types are strongly-typed ints (enum classes).

…prite#3904) - Now we have a view::RealRange (with real frames, fr_t) and a view::VirtualRange (with virtual frames, columns, col_t). The timeline uses a virtual range internally and makes the conversion to a real range when it's appropiated (e.g. to execute an old DocRange operation using ranges with real frames). - Added Context::range() to access to the real selected range instead of accessing directly to the timeline. In this way commands that were using the DocRange can access to the real range using the context, instead of the timeline's virtual range. - Added a new ShowTagTimelineAdapter that can show just one tag in the timeline filtering out/hiding all other frames/tags.

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-04-23T18:58:05Z

src/app/ui/timeline/timeline.h

@@ -289,13 +320,13 @@ namespace app {
 const bool is_disabled = false);
 void drawTop(ui::Graphics* g);
 void drawHeader(ui::Graphics* g);
- void drawHeaderFrame(ui::Graphics* g, const frame_t frame);
+ void drawHeaderFrame(ui::Graphics* g, const col_t col);


warning: parameter 'col' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

void drawHeaderFrame(ui::Graphics* g, const col_t col);

void drawHeaderFrame(ui::Graphics* g, col_t col);

aseprite-bot · 2024-04-23T18:58:05Z

src/app/ui/timeline/timeline.h

 void drawLayer(ui::Graphics* g, const layer_t layerIdx);
 void drawCel(ui::Graphics* g,
- const layer_t layerIdx, const frame_t frame,
+ const layer_t layerIdx, const col_t col,


warning: parameter 'layerIdx' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const layer_t layerIdx, const col_t col,

layer_t layerIdx, const col_t col,

aseprite-bot · 2024-04-23T18:58:06Z

src/app/ui/timeline/timeline.h

 void drawLayer(ui::Graphics* g, const layer_t layerIdx);
 void drawCel(ui::Graphics* g,
- const layer_t layerIdx, const frame_t frame,
+ const layer_t layerIdx, const col_t col,


warning: parameter 'col' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const layer_t layerIdx, const col_t col,

const layer_t layerIdx, col_t col,

aseprite-bot · 2024-04-23T18:58:06Z

src/app/ui/timeline/timeline.h

 const Cel* cel, const DrawCelData* data);
 void drawCelLinkDecorators(ui::Graphics* g, const gfx::Rect& bounds,
- const Cel* cel, const frame_t frame,
+ const Cel* cel, const col_t col,


warning: parameter 'col' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const Cel* cel, const col_t col,

const Cel* cel, col_t col,

aseprite-bot · 2024-04-23T18:58:06Z

src/app/ui/timeline/timeline.h

@@ -314,10 +345,14 @@
 gfx::Rect getPartBounds(const Hit& hit) const;
 gfx::Rect getRangeBounds(const Range& range) const;
 gfx::Rect getRangeClipBounds(const Range& range) const;
+ int getFrameXPos(const col_t frame) const;


warning: parameter 'frame' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

int getFrameXPos(const col_t frame) const;

int getFrameXPos(col_t frame) const;

aseprite-bot · 2024-04-23T18:58:07Z

src/app/util/clipboard.h

@@ -64,7 +67,7 @@ namespace app {
 void cut(ContextWriter& context);
 void copy(const ContextReader& context);
 void copyMerged(const ContextReader& context);
- void copyRange(const ContextReader& context, const DocRange& range);
+ void copyRange(const ContextReader& context, const view::Range& range);


warning: function 'app::Clipboard::copyRange' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

void copyRange(const ContextReader& context, const view::Range& range); ^

Additional context

src/app/util/clipboard.cpp:399: the definition seen here

void Clipboard::copyRange(const ContextReader& reader, const DocRange& range) ^

src/app/util/clipboard.h:69: differing parameters are named here: ('context'), in definition: ('reader')

void copyRange(const ContextReader& context, const view::Range& range); ^

aseprite-bot · 2024-04-23T18:58:07Z

src/dio/aseprite_decoder.cpp

@@ -1161,7 +1161,8 @@ void AsepriteDecoder::readTagsChunk(doc::Tags* tags)
 }

 int repeat = read16(); // Number of times we repeat this tag
- read16(); // 6 reserved bytes
+ int flags = read8(); // Flags


warning: variable 'flags' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int flags = read8(); // Flags

int const flags = read8(); // Flags

aseprite-bot · 2024-04-23T18:58:07Z

src/doc/cels_range.cpp

+CelList CelsRange::toList()
+{
+ CelList list;
+ int n = size();


warning: variable 'n' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int n = size();

int const n = size();

aseprite-bot · 2024-04-23T18:58:07Z

src/view/timeline_adapter.h

+ else
+ return kNoCol;


warning: do not use 'else' after 'return' [readability-else-after-return]

Suggested change

else

return kNoCol;

return kNoCol;

aseprite-bot · 2024-04-23T18:58:07Z

src/view/utils.cpp

+ if (range.enabled()) {
+ doc::SelectedFrames frames;
+ for (auto frame : realRange.selectedFrames()) {
+ col_t col = adapter->toColFrame(fr_t(frame));


warning: variable 'col' of type 'col_t' can be declared 'const' [misc-const-correctness]

Suggested change

col_t col = adapter->toColFrame(fr_t(frame));

col_t const col = adapter->toColFrame(fr_t(frame));

dacap self-assigned this Apr 9, 2024

aseprite-bot reviewed Apr 9, 2024

View reviewed changes

dacap and others added 6 commits April 23, 2024 15:44

Add support for variable frame width in Timeline

a934dd6

Add collapse tags command (only logic side, aseprite#920)

84b2b87

Initial refactor to create the view module (aseprite#3904)

f47d790

* Moved app::DocRange to view::Range * Moved range_utils to view/cels.cpp * Moved layer_utils to view/layers.cpp * Created more functions in app::Site to access selected cels (they use the functions from view/cels.h)

Fix compilation when ENABLE_UI=OFF

248dae6

dacap force-pushed the view branch from be2a0f9 to 248dae6 Compare April 23, 2024 18:45

aseprite-bot reviewed Apr 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new timeline with collapsed/hidden tags #4409

Implement new timeline with collapsed/hidden tags #4409

dacap commented Apr 9, 2024

aseprite-bot left a comment

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot Apr 9, 2024

aseprite-bot left a comment

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

aseprite-bot Apr 23, 2024

	view::RealRange range = context->range();
	view::RealRange const range = context->range();

	std::string id = params.get("id");
	std::string const id = params.get("id");

	return (tag && tag->isCollapsed());
	return ((tag != nullptr) && tag->isCollapsed());

	view::col_t Timeline::getFrameInXPos(const int x) const
	view::col_t Timeline::getFrameInXPos(const int x)

	view::col_t Timeline::calcTagVisibleToFrame(Tag* tag) const
	view::col_t Timeline::calcTagVisibleToFrame(Tag* tag)

	void drawHeaderFrame(ui::Graphics* g, const col_t col);
	void drawHeaderFrame(ui::Graphics* g, col_t col);

	const layer_t layerIdx, const col_t col,
	layer_t layerIdx, const col_t col,

	const layer_t layerIdx, const col_t col,
	const layer_t layerIdx, col_t col,

	int getFrameXPos(const col_t frame) const;
	int getFrameXPos(col_t frame) const;

	int flags = read8(); // Flags
	int const flags = read8(); // Flags

	col_t col = adapter->toColFrame(fr_t(frame));
	col_t const col = adapter->toColFrame(fr_t(frame));

Implement new timeline with collapsed/hidden tags #4409

Are you sure you want to change the base?

Implement new timeline with collapsed/hidden tags #4409

Conversation

dacap commented Apr 9, 2024

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment