People: Improve edit UI and stability #22

frontend/src/dialog/photo/people.vue

@@ -31,7 +31,7 @@
             <v-card-actions class="card-details pa-0">
               <v-layout v-if="marker.Score < 30" row wrap align-center>
                 <v-flex xs6 class="text-xs-center pa-0">
-                  <v-btn color="transparent"
+                  <v-btn color="transparent" :disabled="busy"
                          large depressed block :round="false"
                          class="action-archive text-xs-center"
                          :title="$gettext('Reject')" @click.stop="reject(marker)">
@@ -39,7 +39,7 @@
                   </v-btn>
                 </v-flex>
                 <v-flex xs6 class="text-xs-center pa-0">
-                  <v-btn color="transparent"
+                  <v-btn color="transparent" :disabled="busy"
                          large depressed block :round="false"
                          class="action-approve text-xs-center"
                          :title="$gettext('Approve')" @click.stop="confirm(marker)">
@@ -52,6 +52,7 @@
                   <v-text-field
                       v-model="marker.Name"
                       :rules="[textRule]"
+                      :disabled="busy"
                       browser-autocomplete="off"
                       class="input-name pa-0 ma-0"
                       hide-details
@@ -83,6 +84,7 @@ export default {
   },
   data() {
     return {
+      busy: false,
       markers: this.model.getMarkers(true),
       imageUrl: this.model.thumbnailUrl("fit_720"),
       disabled: !this.$config.feature("edit"),
@@ -133,15 +135,18 @@ export default {
     },
     reject(marker) {
       marker.Invalid = true;
-      this.model.updateMarker(marker);
+      this.busy = true;
+      this.model.updateMarker(marker).finally(() => this.busy = false);
     },
     confirm(marker) {
       marker.Score = 100;
       marker.Invalid = false;
-      this.model.updateMarker(marker);
+      this.busy = true;
+      this.model.updateMarker(marker).finally(() => this.busy = false);
     },
     clearSubject(marker) {
-      this.model.clearMarkerSubject(marker);
+      this.busy = true;
+      this.model.clearMarkerSubject(marker).finally(() => this.busy = false);
     },
     updateName(marker) {
       // Don't save empty name.
@@ -150,8 +155,8 @@ export default {
       }
 
       marker.SubjectSrc = src.Manual;
-
+      this.busy = true;
-      this.model.updateMarker(marker);
+      this.model.updateMarker(marker).finally(() => this.busy = false);
     },
   },
 };

frontend/src/model/photo.js

@@ -848,8 +848,6 @@ export class Photo extends RestModel {
       return Promise.reject("invalid marker id");
     }
 
-    marker.MarkerSrc = src.Manual;
-
     const file = this.mainFile();
 
     if (!file || !file.UID) {

internal/entity/marker.go

@@ -3,13 +3,12 @@ package entity
 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 	"time"
 
+	"github.com/photoprism/photoprism/internal/face"
 	"github.com/photoprism/photoprism/internal/form"
 	"github.com/photoprism/photoprism/pkg/txt"
-	"github.com/ulule/deepcopier"
-
-	"github.com/photoprism/photoprism/internal/face"
 )
 
 const (
@@ -92,19 +91,34 @@ func (m *Marker) Update(attr string, value interface{}) error {
 
 // SaveForm updates the entity using form data and stores it in the database.
 func (m *Marker) SaveForm(f form.Marker) error {
-	if err := deepcopier.Copy(m).From(f); err != nil {
+	changed := false
-		return err
+
+	if m.MarkerInvalid != f.MarkerInvalid {
+		m.MarkerInvalid = f.MarkerInvalid
+		changed = true
 	}
 
-	if f.MarkerName != "" {
+	if !m.MarkerInvalid && m.Score < 100 {
-		m.MarkerName = txt.Title(txt.Clip(f.MarkerName, txt.ClipKeyword))
+		m.Score = 100
+		changed = true
 	}
 
-	if err := m.SyncSubject(true); err != nil {
+	if f.SubjectSrc == SrcManual && strings.TrimSpace(f.MarkerName) != "" {
-		return err
+		m.SubjectSrc = SrcManual
+		m.MarkerName = txt.Title(txt.Clip(f.MarkerName, txt.ClipDefault))
+
+		if err := m.SyncSubject(true); err != nil {
+			return err
+		}
+
+		changed = true
 	}
 
-	return m.Save()
+	if changed {
+		return m.Save()
+	}
+
+	return nil
 }
 
 // SetFace sets a new face for this marker.
@@ -118,11 +132,8 @@ func (m *Marker) SetFace(f *Face) (updated bool, err error) {
 	}
 
 	// Any reason we don't want to set a new face for this marker?
-	if m.SubjectSrc != SrcManual || f.SubjectUID == m.SubjectUID {
+	if m.SubjectSrc != SrcManual || f.SubjectUID == "" || m.SubjectUID == "" || f.SubjectUID == m.SubjectUID {
 		// Don't skip if subject wasn't set manually, or subjects match.
-	} else if f.SubjectUID != "" && m.SubjectUID == "" {
-		log.Debugf("faces: rejected subject %s for marker %d with unknown subject, source %s", txt.Quote(f.SubjectUID), m.ID, m.SubjectSrc)
-		return false, nil
 	} else if reported, err := f.ReportCollision(m.Embeddings()); err != nil {
 		return false, err
 	} else if reported {
@@ -217,7 +228,7 @@ func (m *Marker) SyncSubject(updateRelated bool) error {
 		Updates(Values{"SubjectUID": m.SubjectUID, "SubjectSrc": SrcAuto}).Error; err != nil {
 		return fmt.Errorf("%s (update related markers)", err)
 	} else {
-		log.Debugf("marker: matched subject %s with face %s", subj.SubjectName, m.FaceID)
+		log.Debugf("marker: matched %s with %s", subj.SubjectName, m.FaceID)
 	}
 
 	return nil

internal/entity/subject.go

@@ -27,7 +27,7 @@ type Subject struct {
 	SubjectType        string          `gorm:"type:VARBINARY(8);" json:"Type" yaml:"Type"`
 	SubjectSrc         string          `gorm:"type:VARBINARY(8);" json:"Src" yaml:"Src"`
 	SubjectSlug        string          `gorm:"type:VARBINARY(255);index;" json:"Slug" yaml:"-"`
-	SubjectName        string          `gorm:"type:VARCHAR(255);index;" json:"Name" yaml:"Name"`
+	SubjectName        string          `gorm:"type:VARCHAR(255);unique_index;" json:"Name" yaml:"Name"`
 	SubjectDescription string          `gorm:"type:TEXT;" json:"Description" yaml:"Description,omitempty"`
 	SubjectNotes       string          `gorm:"type:TEXT;" json:"Notes,omitempty" yaml:"Notes,omitempty"`
 	Favorite           bool            `json:"Favorite" yaml:"Favorite,omitempty"`
@@ -44,8 +44,8 @@ type Subject struct {
 // UnknownPerson can be used as a placeholder for unknown people.
 var UnknownPerson = Subject{
 	SubjectUID:  "j000000000000000",
-	SubjectSlug: "unknown",
+	SubjectSlug: "",
-	SubjectName: "Unknown",
+	SubjectName: "",
 	SubjectType: SubjectPerson,
 	SubjectSrc:  SrcDefault,
 	Favorite:    false,
@@ -59,7 +59,7 @@ func CreateUnknownPerson() {
 
 // TableName returns the entity database table name.
 func (Subject) TableName() string {
-	return "subjects_dev4"
+	return "subjects_dev5"
 }
 
 // BeforeCreate creates a random UID if needed before inserting a new row to the database.
@@ -77,13 +77,14 @@ func NewSubject(name, subjectType, subjectSrc string) *Subject {
 		subjectType = SubjectPerson
 	}
 
-	if name == "" {
-		name = UnknownName
-	}
-
 	subjectName := txt.Title(txt.Clip(name, txt.ClipDefault))
 	subjectSlug := slug.Make(txt.Clip(name, txt.ClipSlug))
 
+	// Name is required.
+	if subjectName == "" || subjectSlug == "" {
+		return nil
+	}
+
 	result := &Subject{
 		SubjectSlug: subjectSlug,
 		SubjectName: subjectName,
@@ -144,7 +145,7 @@ func (m *Subject) Updates(values interface{}) error {
 func FirstOrCreateSubject(m *Subject) *Subject {
 	result := Subject{}
 
-	if err := UnscopedDb().Where("subject_type = ? AND subject_slug = ?", m.SubjectType, m.SubjectSlug).First(&result).Error; err == nil {
+	if err := UnscopedDb().Where("subject_name LIKE ?", m.SubjectName).First(&result).Error; err == nil {
 		return &result
 	} else if createErr := m.Create(); createErr == nil {
 		if !m.Hidden && m.SubjectType == SubjectPerson {
@@ -156,10 +157,10 @@ func FirstOrCreateSubject(m *Subject) *Subject {
 		}
 
 		return m
-	} else if err := UnscopedDb().Where("subject_type = ? AND subject_slug = ?", m.SubjectType, m.SubjectSlug).First(&result).Error; err == nil {
+	} else if err := UnscopedDb().Where("subject_name LIKE ?", m.SubjectName).First(&result).Error; err == nil {
 		return &result
 	} else {
-		log.Errorf("subject: %s (find or create %s)", createErr, m.SubjectSlug)
+		log.Errorf("subject: %s while creating %s", createErr, txt.Quote(m.SubjectName))
 	}
 
 	return nil
@@ -173,13 +174,7 @@ func FindSubject(s string) *Subject {
 
 	result := Subject{}
 
-	db := Db()
+	db := Db().Where("subject_uid = ?", s)
-
-	if rnd.IsPPID(s, 'j') {
-		db = db.Where("subject_uid = ?", s)
-	} else {
-		db = db.Where("subject_slug = ?", slug.Make(txt.Clip(s, txt.ClipSlug)))
-	}
 
 	if err := db.First(&result).Error; err != nil {
 		return nil

internal/entity/subject_test.go

@@ -122,18 +122,32 @@ func TestSubject_Restore(t *testing.T) {
 func TestFindSubject(t *testing.T) {
 	t.Run("success", func(t *testing.T) {
 		m := NewSubject("Find Me", SubjectPerson, SrcAuto)
-		err := m.Save()
+
-		if err != nil {
+		if err := m.Save(); err != nil {
 			t.Fatal(err)
 		}
-		found := FindSubject("find me")
+
-		assert.Equal(t, "Find Me", found.SubjectName)
+		if s := FindSubject(m.SubjectName); s != nil {
+			t.Fatal("result must be nil")
+		}
+
+		if s := FindSubject(m.SubjectUID); s != nil {
+			assert.Equal(t, "Find Me", s.SubjectName)
+		} else {
+			t.Fatal("result must not be nil")
+		}
+	})
+	t.Run("unknown person", func(t *testing.T) {
+		if s := FindSubject(UnknownPerson.SubjectUID); s != nil {
+			assert.Equal(t, "", s.SubjectName)
+		} else {
+			t.Fatal("result must not be nil")
+		}
 	})
 	t.Run("nil", func(t *testing.T) {
 		r := FindSubject("XXX")
 		assert.Nil(t, r)
 	})
-
 }
 
 func TestSubject_Links(t *testing.T) {

internal/form/marker.go

@@ -4,13 +4,8 @@ import "github.com/ulule/deepcopier"
 
 // Marker represents an image marker edit form.
 type Marker struct {
-	MarkerType    string `json:"Type"`
-	MarkerSrc     string `json:"Src"`
-	MarkerName    string `json:"Name"`
-	SubjectUID    string `json:"SubjectUID"`
 	SubjectSrc    string `json:"SubjectSrc"`
-	FaceID        string `json:"FaceID"`
+	MarkerName    string `json:"Name"`
-	Score         int    `json:"Score"`
 	MarkerInvalid bool   `json:"Invalid"`
 }
 

internal/form/marker_test.go

@@ -9,22 +9,12 @@ import (
 func TestNewMarker(t *testing.T) {
 	t.Run("success", func(t *testing.T) {
 		var m = struct {
-			MarkerType    string
-			MarkerSrc     string
-			MarkerName    string
-			SubjectUID    string
 			SubjectSrc    string
-			FaceID        string
+			MarkerName    string
-			Score         int
 			MarkerInvalid bool
 		}{
-			MarkerType:    "face",
+			SubjectSrc:    "manual",
-			MarkerSrc:     "image",
 			MarkerName:    "Foo",
-			SubjectUID:    "3h59wvth837b5vyiub35",
-			SubjectSrc:    "meta",
-			FaceID:        "zz",
-			Score:         100,
 			MarkerInvalid: true,
 		}
 
@@ -34,13 +24,8 @@ func TestNewMarker(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		assert.Equal(t, "face", f.MarkerType)
+		assert.Equal(t, "manual", f.SubjectSrc)
-		assert.Equal(t, "image", f.MarkerSrc)
 		assert.Equal(t, "Foo", f.MarkerName)
-		assert.Equal(t, "3h59wvth837b5vyiub35", f.SubjectUID)
-		assert.Equal(t, "zz", f.FaceID)
-		assert.Equal(t, "meta", f.SubjectSrc)
-		assert.Equal(t, 100, f.Score)
 		assert.Equal(t, true, f.MarkerInvalid)
 	})
 }

internal/mutex/mutex.go

@@ -10,9 +10,10 @@ var (
 	SyncWorker  = Busy{}
 	ShareWorker = Busy{}
 	MetaWorker  = Busy{}
+	FacesWorker = Busy{}
 )
 
 // WorkersBusy returns true if any worker is busy.
 func WorkersBusy() bool {
-	return MainWorker.Busy() || SyncWorker.Busy() || ShareWorker.Busy() || MetaWorker.Busy()
+	return MainWorker.Busy() || SyncWorker.Busy() || ShareWorker.Busy() || MetaWorker.Busy() || FacesWorker.Busy()
 }

internal/photoprism/faces.go

@@ -45,11 +45,11 @@ func (w *Faces) Start(opt FacesOptions) (err error) {
 		return fmt.Errorf("facial recognition is disabled")
 	}
 
-	if err := mutex.MainWorker.Start(); err != nil {
+	if err := mutex.FacesWorker.Start(); err != nil {
 		return err
 	}
 
-	defer mutex.MainWorker.Stop()
+	defer mutex.FacesWorker.Stop()
 
 	// Remove invalid reference IDs from markers table.
 	if removed, err := query.RemoveInvalidMarkerReferences(); err != nil {
@@ -60,6 +60,15 @@ func (w *Faces) Start(opt FacesOptions) (err error) {
 		log.Debugf("faces: no invalid references")
 	}
 
+	// Add known marker subjects.
+	if affected, err := query.AddFaceMarkerSubjects(); err != nil {
+		log.Errorf("faces: %s (match markers with subjects)", err)
+	} else if affected > 0 {
+		log.Infof("faces: added %d known marker subjects", affected)
+	} else {
+		log.Debugf("faces: no subjects were missing")
+	}
+
 	// Optimize existing face clusters.
 	if res, err := w.Optimize(); err != nil {
 		return err
@@ -69,15 +78,6 @@ func (w *Faces) Start(opt FacesOptions) (err error) {
 		log.Debugf("faces: no clusters could be merged")
 	}
 
-	// Add known marker subjects.
-	if affected, err := query.AddMarkerSubjects(); err != nil {
-		log.Errorf("faces: %s (match markers with subjects)", err)
-	} else if affected > 0 {
-		log.Infof("faces: added %d known marker subjects", affected)
-	} else {
-		log.Debugf("faces: no subjects were missing")
-	}
-
 	var added entity.Faces
 
 	// Cluster existing face embeddings.
@@ -108,7 +108,12 @@ func (w *Faces) Start(opt FacesOptions) (err error) {
 
 // Cancel stops the current operation.
 func (w *Faces) Cancel() {
-	mutex.MainWorker.Cancel()
+	mutex.FacesWorker.Cancel()
+}
+
+// Canceled tests if face clustering and matching should be stopped.
+func (w *Faces) Canceled() bool {
+	return mutex.FacesWorker.Canceled() || mutex.MainWorker.Canceled() || mutex.MetaWorker.Canceled()
 }
 
 // Disabled tests if facial recognition is disabled.

internal/photoprism/faces_match.go

@@ -5,7 +5,6 @@ import (
 	"time"
 
 	"github.com/photoprism/photoprism/internal/entity"
-	"github.com/photoprism/photoprism/internal/mutex"
 	"github.com/photoprism/photoprism/internal/query"
 )
 
@@ -38,7 +37,7 @@ func (w *Faces) Match(opt FacesOptions) (result FacesMatchResult, err error) {
 		return result, err
 	}
 
-	limit := 500
+	limit := 100
 	offset := 0
 
 	for {
@@ -53,7 +52,7 @@ func (w *Faces) Match(opt FacesOptions) (result FacesMatchResult, err error) {
 		}
 
 		for _, marker := range markers {
-			if mutex.MainWorker.Canceled() {
+			if w.Canceled() {
 				return result, fmt.Errorf("worker canceled")
 			}
 

internal/query/markers.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 
 	"github.com/photoprism/photoprism/internal/entity"
-	"github.com/photoprism/photoprism/pkg/txt"
 )
 
 // MarkerByID returns a Marker based on the ID.
@@ -74,40 +73,6 @@ func Embeddings(single, unclustered bool) (result entity.Embeddings, err error)
 	return result, nil
 }
 
-// AddMarkerSubjects adds and references known marker subjects.
-func AddMarkerSubjects() (affected int64, err error) {
-	var markers entity.Markers
-
-	if err := Db().
-		Where("face_id <> '' AND subject_uid = '' AND subject_src = ?", entity.SrcAuto).
-		Where("marker_invalid = 0 AND marker_type = ?", entity.MarkerFace).
-		Where("marker_name <> ''").
-		Order("marker_name").
-		Find(&markers).Error; err != nil {
-		return affected, err
-	} else if len(markers) == 0 {
-		return affected, nil
-	}
-
-	for _, m := range markers {
-		if faceId := m.FaceID; faceId == "" {
-			// Do nothing.
-		} else if subj := entity.NewSubject(m.MarkerName, entity.SubjectPerson, entity.SrcMarker); subj == nil {
-			log.Errorf("faces: subject should not be nil - bug?")
-		} else if subj = entity.FirstOrCreateSubject(subj); subj == nil {
-			log.Errorf("faces: failed adding subject %s for marker %d", txt.Quote(m.MarkerName), m.ID)
-		} else if err := m.Updates(entity.Values{"SubjectUID": subj.SubjectUID, "SubjectSrc": entity.SrcAuto}); err != nil {
-			return affected, err
-		} else if err := Db().Model(&entity.Face{}).Where("id = ? AND subject_uid = ''", faceId).Update("SubjectUID", subj.SubjectUID).Error; err != nil {
-			return affected, err
-		} else {
-			affected++
-		}
-	}
-
-	return affected, err
-}
-
 // RemoveInvalidMarkerReferences deletes invalid reference IDs from the markers table.
 func RemoveInvalidMarkerReferences() (removed int64, err error) {
 	// Remove subject and face relationships for invalid markers.

internal/query/markers_test.go

@@ -59,13 +59,6 @@ func TestEmbeddings(t *testing.T) {
 	}
 }
 
-func TestAddMarkerSubjects(t *testing.T) {
-	affected, err := AddMarkerSubjects()
-
-	assert.NoError(t, err)
-	assert.GreaterOrEqual(t, affected, int64(1))
-}
-
 func TestRemoveInvalidMarkerReferences(t *testing.T) {
 	affected, err := RemoveInvalidMarkerReferences()
 

internal/query/subjects.go

@@ -3,6 +3,8 @@ package query
 import (
 	"fmt"
 
+	"github.com/photoprism/photoprism/pkg/txt"
+
 	"github.com/photoprism/photoprism/internal/entity"
 )
 
@@ -10,7 +12,7 @@ import (
 func Subjects(limit, offset int) (result entity.Subjects, err error) {
 	stmt := Db()
 
-	stmt = stmt.Order("subject_slug").Limit(limit).Offset(offset)
+	stmt = stmt.Order("subject_name").Limit(limit).Offset(offset)
 	err = stmt.Find(&result).Error
 
 	return result, err
@@ -26,3 +28,49 @@ func RemoveDanglingMarkerSubjects() (removed int64, err error) {
 
 	return res.RowsAffected, res.Error
 }
+
+// AddFaceMarkerSubjects adds and references known marker subjects.
+func AddFaceMarkerSubjects() (affected int64, err error) {
+	var markers entity.Markers
+
+	if err := Db().
+		Where("subject_uid = '' AND marker_name <> ''").
+		Where("marker_invalid = 0 AND marker_type = ?", entity.MarkerFace).
+		Order("marker_name").
+		Find(&markers).Error; err != nil {
+		return affected, err
+	} else if len(markers) == 0 {
+		return affected, nil
+	}
+
+	var name string
+	var subj *entity.Subject
+
+	for _, m := range markers {
+		if name == m.MarkerName && subj != nil {
+			// Do nothing.
+		} else if subj = entity.NewSubject(m.MarkerName, entity.SubjectPerson, entity.SrcMarker); subj == nil {
+			log.Errorf("faces: subject should not be nil - bug?")
+			continue
+		} else if subj = entity.FirstOrCreateSubject(subj); subj == nil {
+			log.Errorf("faces: failed adding subject %s", txt.Quote(m.MarkerName))
+			continue
+		} else {
+			affected++
+		}
+
+		name = m.MarkerName
+
+		if err := m.Updates(entity.Values{"SubjectUID": subj.SubjectUID}); err != nil {
+			return affected, err
+		}
+
+		if m.FaceID == "" {
+			continue
+		} else if err := Db().Model(&entity.Face{}).Where("id = ? AND subject_uid = ''", m.FaceID).Update("SubjectUID", subj.SubjectUID).Error; err != nil {
+			return affected, err
+		}
+	}
+
+	return affected, err
+}

internal/query/subjects_test.go

@@ -31,3 +31,10 @@ func TestRemoveDanglingMarkerSubjects(t *testing.T) {
 
 	assert.Equal(t, int64(1), affected)
 }
+
+func TestAddFaceMarkerSubjects(t *testing.T) {
+	affected, err := AddFaceMarkerSubjects()
+
+	assert.NoError(t, err)
+	assert.GreaterOrEqual(t, affected, int64(2))
+}

internal/workers/workers.go

@@ -94,7 +94,7 @@ func StartShare(conf *config.Config) {
 	}
 }
 
-// StartShare runs the sync worker once.
+// StartSync runs the sync worker once.
 func StartSync(conf *config.Config) {
 	if !mutex.SyncWorker.Busy() {
 		go func() {