Cleanup for review

app/config/config.cpp

@@ -135,7 +135,7 @@ void Config::SetDefaults()
   SetEntryInternal(QStringLiteral("OnlinePixelFormat"), NodeValue::kInt, VideoParams::kFormatFloat32);
   SetEntryInternal(QStringLiteral("OfflinePixelFormat"), NodeValue::kInt, VideoParams::kFormatFloat16);
 
-  SetEntryInternal(QStringLiteral("MarkerColor"), NodeValue::kInt, ColorCoding::kRed);
+  SetEntryInternal(QStringLiteral("MarkerColor"), NodeValue::kInt, ColorCoding::kGreen);
 }
 
 void Config::Load()

app/panel/timeline/timeline.cpp

@@ -20,8 +20,6 @@
 
 #include "timeline.h"
 
-#include <QApplication>
-
 #include "panel/panelmanager.h"
 #include "panel/project/footagemanagementpanel.h"
 

app/widget/marker/marker.cpp

@@ -23,8 +23,8 @@
 #include <QInputDialog>
 #include <QPainter>
 
-#include "common/qtutils.h"
 #include "config/config.h"
+#include "common/qtutils.h"
 #include "panel/panelmanager.h"
 #include "panel/timeline/timeline.h"
 #include "ui/colorcoding.h"
@@ -58,7 +58,8 @@ void Marker::SetActive(bool active)
 {
   active_ = active;
 
-  // Feels very hacky, might it be better to write some access methods?
+  // Generaly the same functions delete both markers and timeline blocks. This helps ensure that
+  // markers and timeline blocks can never be selected at the same time
   if (active) {
     TimelinePanel *timeline = PanelManager::instance()->MostRecentlyFocused<TimelinePanel>();
     if (timeline) {
@@ -78,9 +79,6 @@ void Marker::paintEvent(QPaintEvent *event)
 {
   QFontMetrics fm = fontMetrics();
 
-  //int text_height = fm.height();
-  //int marker_width_ = QtUtils::QFontMetricsWidth(fm, "H");
-
   int y = marker_height_;
 
   int half_width = marker_width_ / 2;
@@ -114,8 +112,12 @@ void Marker::paintEvent(QPaintEvent *event)
 
   if (!name_.isEmpty()) {
     resize(marker_width_ + fm.horizontalAdvance(name_) + fm.horizontalAdvance(" "), marker_height_);
+
+    // Draw background rectangle
     color.setAlphaF(0.5);
     p.fillRect(x, y, fm.horizontalAdvance(name_) + fm.horizontalAdvance(" ") * 2 + half_width, -marker_height_, color);
+
+    // Draw text
     p.drawText(x + marker_width_, y - half_marker_height, name_);
   } else {
     resize(marker_width_, marker_height_);
@@ -166,14 +168,16 @@ void Marker::mouseMoveEvent(QMouseEvent* e)
 
     rational marker_time = SeekableParent()->ScreenToTime(new_position);
 
-    if (Core::instance()->snapping()) {
-      rational movement;
-      SeekableParent()->GetSnapService()->SnapPoint({marker_time}, &movement);
-      if (!movement.isNull()) {
-        marker_time += movement;
-      }
+    if (SeekableParent()->GetSnapService()) {
+      if (Core::instance()->snapping()) {
+        rational movement;
+        SeekableParent()->GetSnapService()->SnapPoint({marker_time}, &movement);
+        if (!movement.isNull()) {
+          marker_time += movement;
+        }
 
-      new_position = SeekableParent()->TimeToScene(marker_time);
+        new_position = SeekableParent()->TimeToScene(marker_time);
+      }
     }
 
     if (new_position > -marker_width_ / 2 && new_position < SeekableParent()->width() - marker_width_ / 2) {
@@ -198,7 +202,7 @@ void Marker::mouseReleaseEvent(QMouseEvent* e)
 
 SeekableWidget* Marker::SeekableParent()
 {
-  return static_cast<SeekableWidget *>(parent());
+  return dynamic_cast<SeekableWidget *>(parent());
 }
 
 void Marker::ShowContextMenu()

app/widget/marker/marker.h

@@ -57,8 +57,6 @@ class Marker : public QWidget {
   virtual void mouseReleaseEvent(QMouseEvent* event) override;
 
  signals:
-  //void MouseClicked();
-  //void MouseDoubleClicked();
   void ColorChanged(int c);
   void markerSelected(Marker* marker);
   void ActiveChanged(bool active);

app/widget/marker/markercopypaste.cpp

@@ -77,8 +77,6 @@ void MarkerCopyPasteService::PasteMarkersFromClipboard(TimelineMarkerList* list,
   QXmlStreamReader reader(clipboard);
   uint data_version = 0;
 
-  QVector<TimelineMarker*> pasted_markers;
-
   while (XMLReadNextStartElement(&reader)) {
     if (reader.name() == QStringLiteral("olive")) {
       while (XMLReadNextStartElement(&reader)) {

app/widget/timelinewidget/tool/pointer.cpp

@@ -136,6 +136,7 @@ void PointerTool::MousePress(TimelineViewMouseEvent *event)
     drag_global_start_ = QCursor::pos();
   }
 
+  // If we click anywhere other than a marker, deselect all markers
   parent()->ruler()->DeselectAllMarkers();
 }
 

app/widget/timelinewidget/view/timelineview.cpp

@@ -62,6 +62,7 @@ TimelineView::TimelineView(Qt::Alignment vertical_alignment, QWidget *parent) :
 
 void TimelineView::mousePressEvent(QMouseEvent *event)
 {
+  // If we click on marker, jump to that point in the timeline
   foreach (QRectF rect, clip_marker_positions_.values()) {
     if (rect.contains(mapToScene(event->pos()))) {
       TimelinePanel *timeline = PanelManager::instance()->MostRecentlyFocused<TimelinePanel>();
@@ -546,8 +547,7 @@ void TimelineView::DrawBlock(QPainter *painter, bool foreground, Block *block, q
                   if (!marker->name().isEmpty()) {
                     int length = fm.horizontalAdvance(marker->name());
                     if (iterator.hasNext()) {
-                      if (TimeToScene(iterator.peekNext()->time().in()) - TimeToScene(marker->time().out()) <
-                          (double)length) {
+                      if (TimeToScene(iterator.peekNext()->time().in()) - TimeToScene(marker->time().out()) < (double)length) {
                         draw_name = false;
                       }
                     }

app/widget/timeruler/seekablewidget.cpp

@@ -26,8 +26,8 @@
 
 #include "common/qtutils.h"
 #include "core.h"
-#include "widget/timebased/timebasedwidget.h"
 #include "widget/marker/markerundo.h"
+#include "widget/timebased/timebasedwidget.h"
 
 namespace olive {
 
@@ -46,7 +46,6 @@ SeekableWidget::SeekableWidget(QWidget* parent) :
   playhead_width_ = QtUtils::QFontMetricsWidth(fm, "H");
 
   setContextMenuPolicy(Qt::CustomContextMenu);
-
 }
 
 void SeekableWidget::ConnectTimelinePoints(TimelinePoints *points)
@@ -211,6 +210,14 @@ void SeekableWidget::addMarker(TimelineMarker* marker)
     Marker *marker_widget = new Marker(this);
     marker_map_.insert(marker, marker_widget);
 
+    /*
+    Markers are stored as TimelineMarkers and represented in the UI as Markers. As a single
+    TimelineMarker can be represented in various views it is necessary to make sure all instances
+    of the TimelineMarker's (UI) Markers are kept in sync. To do this, whenever a Marker is updated
+    in some way, it signals that change to the relevant TimelineMarker which in turn broadcasts
+    that update out to all the relevant Markers.
+    */
+
     connect(marker_widget, &Marker::ColorChanged, this, &SeekableWidget::SetMarkerColor);
     connect(marker, &TimelineMarker::ColorChanged, marker_widget, &Marker::SetColor);
 
@@ -327,10 +334,7 @@ void SeekableWidget::DrawTimelinePoints(QPainter* p, int marker_bottom)
         continue;
       }
 
-      if (marker->time().length() == 0) {
-        // Single point in time marker
-        //DrawPlayhead(p, marker_left, marker_bottom);
-      } else {
+      if (marker->time().length() != 0) {
         // Marker range
         int rect_left = qMax(0, marker_left);
         int rect_right = qMin(width(), marker_right);

app/widget/timeruler/timeruler.h

@@ -27,7 +27,6 @@
 #include "common/timerange.h"
 #include "seekablewidget.h"
 #include "render/playbackcache.h"
-#include "widget/marker/marker.h"
 
 namespace olive {